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

Status: VERIFIED FIXED

(Toolkit :: Autocomplete, defect)

Description

[:Gavin Sharp [email: [e-mail]]]

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

[Dão Gottwald [:dao]]

There's a autocomplete-base-popup binding, might be a good idea to add the attribute there.

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #0)
> Shouldn't this popup have level="top"?

yeah, it's better.

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v1.0

Comment 5

[:Gavin Sharp [email: [e-mail]]]

That works, but it would probably be best to put it on autocomplete-base-popup's <content> instead, assuming that works?

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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

[Dão Gottwald [:dao]]

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?

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(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....

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v2.0

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...

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

bug 448929 might be the cause that Patch v2 doesn't work on DOM Inspector.

Comment 11

[:Gavin Sharp [email: [e-mail]]]

Actually, why wouldn't we just set it on both autocomplete-rich-result-popup and
autocomplete-result-popup?

Comment 12

[Dão Gottwald [:dao]]

(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...

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(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.

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v3.0

This works fine, but the future implementations of autocomplete-base-popup also need same code.

Comment 15

[:Gavin Sharp [email: [e-mail]]]

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.

Comment 16

[:Gavin Sharp [email: [e-mail]]]

Though if v3 works perhaps we should just go with that and not worry about other people extending these bindings.

Comment 17

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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 :-(

Comment 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(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.

Comment 19

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

checked-in.

Comment 20

[Dietrich Ayala (:dietrich)]

verified, thanks for fixing so quickly!

Comment 21

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #20)
> verified, thanks for fixing so quickly!

Marking verified based on dietrich's comment.

Comment 22

[Serge Gautherie (:sgautherie)]

(In reply to comment #19)
> checked-in.

http://hg.mozilla.org/mozilla-central/rev/5c4d3ecd834a