Closed Bug 454531 Opened 12 years ago Closed 12 years ago

default autocomplete panel doesn't have level="top"

Categories

(Toolkit :: Autocomplete, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla1.9.1b1

People

(Reporter: Gavin, Assigned: masayuki)

References

Details

(Keywords: regression)

Attachments

(1 file, 2 obsolete files)

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.
Duplicate of this bug: 454527
There's a autocomplete-base-popup binding, might be a good idea to add the attribute there.
(In reply to comment #0)
> Shouldn't this popup have level="top"?

yeah, it's better.
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
Keywords: regression
Blocks: 415960
Attached patch Patch v1.0 (obsolete) — Splinter Review
Attachment #337894 - Flags: review?(gavin.sharp)
That works, but it would probably be best to put it on autocomplete-base-popup's <content> instead, assuming that works?
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.
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?
(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....
Attached patch Patch v2.0 (obsolete) — Splinter Review
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...
bug 448929 might be the cause that Patch v2 doesn't work on DOM Inspector.
Actually, why wouldn't we just set it on both autocomplete-rich-result-popup and
autocomplete-result-popup?
Attachment #337894 - Attachment is obsolete: true
Attachment #337894 - Flags: review?(gavin.sharp)
(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...
(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.
Attached patch Patch v3.0Splinter Review
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)
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.
Though if v3 works perhaps we should just go with that and not worry about other people extending these bindings.
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 :-(
(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.
Attachment #337939 - Flags: review?(gavin.sharp) → review+
checked-in.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
verified, thanks for fixing so quickly!
(In reply to comment #20)
> verified, thanks for fixing so quickly!

Marking verified based on dietrich's comment.
Status: RESOLVED → VERIFIED
(In reply to comment #19)
> checked-in.

http://hg.mozilla.org/mozilla-central/rev/5c4d3ecd834a
Blocks: 561116
Target Milestone: --- → mozilla1.9.1b1
You need to log in before you can comment on or make changes to this bug.