Closed
Bug 454531
Opened 16 years ago
Closed 16 years ago
default autocomplete panel doesn't have level="top"
Categories
(Toolkit :: Autocomplete, defect)
Toolkit
Autocomplete
Tracking
()
VERIFIED
FIXED
mozilla1.9.1b1
People
(Reporter: Gavin, Assigned: masayuki)
References
Details
(Keywords: regression)
Attachments
(1 file, 2 obsolete files)
(deleted),
patch
|
Gavin
:
review+
|
Details | Diff | Splinter Review |
For "normal" autocomplete widgets, the binding creates it's own popup:
http://hg.mozilla.org/mozilla-central/annotate/50ad9a5fd783/toolkit/content/widgets/autocomplete.xml#l104
Shouldn't this popup have level="top"? See also bug 415960 comment 35.
Comment 2•16 years ago
|
||
There's a autocomplete-base-popup binding, might be a good idea to add the attribute there.
Assignee | ||
Comment 3•16 years ago
|
||
(In reply to comment #0)
> Shouldn't this popup have level="top"?
yeah, it's better.
Assignee | ||
Updated•16 years ago
|
Assignee | ||
Comment 4•16 years ago
|
||
Attachment #337894 -
Flags: review?(gavin.sharp)
Reporter | ||
Comment 5•16 years ago
|
||
That works, but it would probably be best to put it on autocomplete-base-popup's <content> instead, assuming that works?
Assignee | ||
Comment 6•16 years ago
|
||
No, that doesn't work. We need to add the attribute to each implementation. I.e., I also need to add the attr to both autocomplete-rich-result-popup and autocomplete-result-popup.
Comment 7•16 years ago
|
||
In fact, doing this in the autocomplete-base-popup binding will work in more
cases, i.e. also for autocomplete popups that are associated through the
autocompletepopup attribute.
(In reply to comment #6)
> No, that doesn't work. We need to add the attribute to each implementation.
> I.e., I also need to add the attr to both autocomplete-rich-result-popup and
> autocomplete-result-popup.
Do it in the constructor?
Assignee | ||
Comment 8•16 years ago
|
||
(In reply to comment #7)
> (In reply to comment #6)
> > No, that doesn't work. We need to add the attribute to each implementation.
> > I.e., I also need to add the attr to both autocomplete-rich-result-popup and
> > autocomplete-result-popup.
>
> Do it in the constructor?
Oh, I'm trying....
Assignee | ||
Comment 9•16 years ago
|
||
It's strange... This patch works fine at autocomplete for form controls, search bar and location bar. However, this doesn't work fine on the location bar of DOM Inspector...
Assignee | ||
Comment 10•16 years ago
|
||
bug 448929 might be the cause that Patch v2 doesn't work on DOM Inspector.
Reporter | ||
Updated•16 years ago
|
Attachment #337932 -
Flags: review+
Reporter | ||
Comment 11•16 years ago
|
||
Actually, why wouldn't we just set it on both autocomplete-rich-result-popup and
autocomplete-result-popup?
Reporter | ||
Updated•16 years ago
|
Attachment #337894 -
Attachment is obsolete: true
Attachment #337894 -
Flags: review?(gavin.sharp)
Comment 12•16 years ago
|
||
(In reply to comment #11)
> Actually, why wouldn't we just set it on both autocomplete-rich-result-popup
> and autocomplete-result-popup?
Any binding that extends them with its own <content/> would have to catch up...
Assignee | ||
Comment 13•16 years ago
|
||
(In reply to comment #11)
> Actually, why wouldn't we just set it on both autocomplete-rich-result-popup
> and
> autocomplete-result-popup?
It's most simple way, but looks like not smart :-p
Patch v2 doesn't fix this bug.
Assignee | ||
Comment 14•16 years ago
|
||
This works fine, but the future implementations of autocomplete-base-popup also need same code.
Attachment #337932 -
Attachment is obsolete: true
Attachment #337939 -
Flags: review?(gavin.sharp)
Reporter | ||
Comment 15•16 years ago
|
||
v2.0 doesn't work? I guess the constructor for the Firefox urlbar panels isn't fired until right before we open the popup (since the element is hidden before that: http://hg.mozilla.org/mozilla-central/annotate/8a3c07d864ba/browser/base/content/urlbarBindings.xml#l288 ), so I guess it's not being added soon enough. You could probably just keep the hardcoded attribute for those panels to work around that.
Reporter | ||
Comment 16•16 years ago
|
||
Though if v3 works perhaps we should just go with that and not worry about other people extending these bindings.
Assignee | ||
Comment 17•16 years ago
|
||
I guess that the constructor is fired *after* nsMenuPopupFrame creating the native widget for it at DOM Inspector case. If so, by bug 448929, the panel doesn't refer the level attribute :-(
Assignee | ||
Comment 18•16 years ago
|
||
(In reply to comment #16)
> Though if v3 works perhaps we should just go with that and not worry about
> other people extending these bindings.
I agree. It's best way for now.
Reporter | ||
Updated•16 years ago
|
Attachment #337939 -
Flags: review?(gavin.sharp) → review+
Assignee | ||
Comment 19•16 years ago
|
||
checked-in.
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Comment 20•16 years ago
|
||
verified, thanks for fixing so quickly!
Comment 21•16 years ago
|
||
(In reply to comment #20)
> verified, thanks for fixing so quickly!
Marking verified based on dietrich's comment.
Status: RESOLVED → VERIFIED
Comment 22•15 years ago
|
||
Blocks: 561116
Target Milestone: --- → mozilla1.9.1b1
You need to log in
before you can comment on or make changes to this bug.
Description
•