1513469 - First entry in the urlbar dropdown is no longer highlighted by default

Status: RESOLVED FIXED

(Firefox :: Address Bar, defect, P1)

Description

[Bruce]

Attachment: No highlight.gif

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:66.0) Gecko/20100101 Firefox/66.0

Steps to reproduce:

Type `test` in the address bar without quotes.


Actual results:

The `[lens] test` line did not have the usual blue background. If I press down-arrow, followed by up-arrow, it still does not get the blue background.


Expected results:

The `[lens] test` line should have a blue background.

Comment 1

[Marco Bonardo [:mak]]

It looks like something regressed this very recently

Comment 2

[Marco Bonardo [:mak]]

Regression range:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=53fd96ca5aa4298054f581ca846ea2cccbe76085&tochange=6915aee134cb2b579b67e7718579c7a59b15e1d8

Comment 3

[Marco Bonardo [:mak]]

off-hand it looks like  it may be due to bug 1492326. Neil, any idea?

Comment 4

[Neil Deakin]

It looks like some code is trying to retrieve richlistbox.itemChildren when that first item has not yet been initialized (has no binding). The old version of "itemChildren" would skip that item, whereas the new version will just return all richlistitems. The latter is probably more correct considering that "itemChildren" should return its children.

This is a stacktrace of the caller:

chrome://global/content/bindings/richlistbox.xml 601 get_itemChildren
chrome://global/content/bindings/richlistbox.xml 221 getItemAtIndex
chrome://global/content/bindings/richlistbox.xml 163 set_selectedIndex
chrome://global/content/bindings/autocomplete.xml 760 set_selectedIndex
chrome://browser/content/urlbarBindings.xml 2483 onResultsAdded
chrome://global/content/bindings/autocomplete.xml 1059 _appendCurrentResult
chrome://global/content/bindings/autocomplete.xml 864 _invalidate
chrome://browser/content/urlbarBindings.xml 2194 _openAutocompletePopup
chrome://browser/content/urlbarBindings.xml 2107 openAutocompletePopup
chrome://global/content/bindings/autocomplete.xml 344 openPopup
chrome://global/content/bindings/autocomplete.xml 90 set_popupOpen

Comment 5

[Marco Bonardo [:mak]]

I'm not sure I understand, we set the selection to the item at index 0 (see in urlbarbindings this.input.controller.setInitiallySelectedIndex(0)), after that item has been appended to the dom. What do you mean with "the old version would skip that item", the item at 0 was properly selected in the old version, if the new version returns all the items, it should still select the item at 0.
Are you saying we set the selection on a different item than the one we expect to be at index 0?

Comment 6

[Neil Deakin]

With the old code, it looks like setInitiallySelectedIndex initially fails, as the first item has a node, but is not present in itemChildren, as described above. setInitiallySelectedIndex then gets called again on a later call to invalidate() and is successful in assigning to the first index.

With the new code, the first call to setInitiallySelectedIndex succeeds, but because the binding for that item isn't yet present, the selected attribute/property never gets assigned.

I'm not familiar enough with the mechanism that is calling setInitiallySelectedIndex to know if it should/should not be reseting something that causes it to get called again, or perhaps the richlistbox should handle the element as not yet selected.

Comment 7

[Marco Bonardo [:mak]]

Let me see if I got this correctly.
We try to set richlistitem.selected when the binding is not attached yet, due to this we end up setting an expando on it that will prevent the "selected" property of the binding from working properly once attached.
Indeed the item even later is basically stuck and can't be selected anymore.

the only solution I found so far is to delay onResultsAdded, so that the binding is attached by the time we try to set selection. This is not great though (may be ok-ish temporarily, considered we are moving off of this code), the richlistbox should not break its children, since we have a richlistbox, we should be able to call selectedIndex at any time, without it breaking its internals.

Comment 8

[Marco Bonardo [:mak]]

Or, we could remove the "selected" property and just use plain attributes. The property seems to be used only inside richlistbox anyway

Comment 9

[Marco Bonardo [:mak]]

Hm but it implements nsIDOMXULSelectControlItemElement :(

Comment 10

[Marco Bonardo [:mak]]

I'll test a patch delaying onResultsAdded on Try, if you have better ideas to fix richlistbox, they are welcome, but we need to address this regression sooner than later.

Comment 11

[Neil Deakin]

We can also just back out the one-line itemChildren change from richlistbox.xml until we find a better fix.

Comment 12

[Marco Bonardo [:mak]]

Attachment: Bug 1513469 - First entry in the urlbar dropdown is no longer highlighted by default. r=adw!

We must delay setting the selected index, otherwise we may end dealing with
richlistitems without an applied binding, and the richlistbox may break them
permanently by setting a "selected" expando.

Comment 13

[Marco Bonardo [:mak]]

let's try with this location bar fix, we should probably still track the richlistbox issue apart in toolkit, because I still don't think it's ok that a binding can break its internals.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/974da7ec60a9
First entry in the urlbar dropdown is no longer highlighted by default. r=adw

Comment 15

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/974da7ec60a9

Comment 16

[Mohammad Maruf Rahman]

I have reproduced this bug with Nightly 66.0a1 (2018-12-12) in Windows 10, 64 Bit. This bug's fix is verified with latest Nightly!

Build ID : 20190120213632
User Agent : Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:66.0) Gecko/20100101 Firefox/66.0