Closed Bug 1513469 Opened 4 years ago Closed 4 years ago

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

Categories

(Firefox :: Address Bar, defect, P1)

66 Branch
defect

Tracking

()

RESOLVED FIXED
Firefox 66
Tracking Status
firefox-esr60 --- unaffected
firefox64 --- unaffected
firefox65 --- unaffected
firefox66 + fixed

People

(Reporter: bruce.bugz, Assigned: mak)

References

(Depends on 1 open bug)

Details

(Keywords: regression, Whiteboard: [fxsearch])

Attachments

(2 files)

Attached image 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.
Component: Untriaged → Address Bar
It looks like something regressed this very recently
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P1
Whiteboard: [fxsearch]
off-hand it looks like  it may be due to bug 1492326. Neil, any idea?
Blocks: 1492326
Flags: needinfo?(enndeakin)
Keywords: regression
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
Flags: needinfo?(enndeakin)
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.
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Depends on: 1514141
Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/autoland/rev/974da7ec60a9
First entry in the urlbar dropdown is no longer highlighted by default. r=adw
https://hg.mozilla.org/mozilla-central/rev/974da7ec60a9
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 66

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

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.