Closed Bug 1513469 Opened 4 years ago Closed 4 years ago
First entry in the urlbar dropdown is no longer highlighted by default
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.
It looks like something regressed this very recently
Status: UNCONFIRMED → NEW
Ever confirmed: true
off-hand it looks like it may be due to bug 1492326. Neil, any idea?
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
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?
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.
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.
Or, we could remove the "selected" property and just use plain attributes. The property seems to be used only inside richlistbox anyway
Hm but it implements nsIDOMXULSelectControlItemElement :(
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.
We can also just back out the one-line itemChildren change from richlistbox.xml until we find a better fix.
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.
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.
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/autoland/rev/974da7ec60a9 First entry in the urlbar dropdown is no longer highlighted by default. r=adw
QA Whiteboard: [bugday-20190116]
QA Whiteboard: [bugday-20190116] → [bugday-20190116] [good first verify]
You need to log in before you can comment on or make changes to this bug.