From https://phabricator.services.mozilla.com/D161776:

There's some notable visual differences between the implementation compared to the design.

jteow, would you mind listing what differences you saw? Just want to make sure we don't miss any of that. Doesn't need to be a comprehensive list.

No problem, here are some differences I saw:

• While highlighted, the meatball button should visually appear within a result rather to the right of. If it's too problematic to implement it like the design, it might be worth mentioning to Ryan since it's a noticeable change.
• Assuming the meatball menu button visually appears within a highlighted result:
  • When the result row is highlighted, the background of the button should change to match the design.
  • On hover and active, the button background and meatball color should match the design.
  • On focus, the button should have a border and the meatball color should have a different color.
    • Additionally, the urlbar result it is associated with should remain highlighted.
• The button itself should be a circle rather than a rounded rectangle.
• While a row that could contain a meatball menu button is not highlighted, the meatball button should not appear.

Backed out for causing bc failures on browser_ext_themes_autocomplete_popup.js.

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=4b53f5a10a764aac54c88fd85d8237fbcd518765&selectedTaskRun=C_75MgmnR0i9xr5ZqCl2Qw.0

Failure log: https://treeherder.mozilla.org/logviewer?job_id=402895338&repo=autoland

Backout link: https://hg.mozilla.org/integration/autoland/rev/1522c76d8612706f2a70025f42bd3ae40730dc78

https://hg.mozilla.org/mozilla-central/rev/95efc81f5d0d

Can we back this out and figure out a better approach? It's interfering with our ability to iterate on the weather suggestion. I didn't file a bug for this but it also removed the border radius on all dynamic results on hover and selection: onboarding tab to search, calculator, and unit converter.

I think you'll need to take some ideas from https://phabricator.services.mozilla.com/D163630 to avoid problems like the border radius and hover gap between rows. It's kind of hard to get right for all the different types of rows we now have, when accounting for buttons, rows with row/group labels, dynamic results, etc.

If you do end up making a new patch, could you flag me for review please? I have a lot of experience with this recently.

Dao, what are you thoughts about backing out and tweaking this? I should've caught the impact to the dynamic types.

One thought I had was to combine Drew's solution of making the urlbarView-row-inner selectable by also moving the constructions of button step to inside of row-inner so that they are encapsulated when inner is selected. However, I might be missing a key reason why buttons remained separate from inner in the first place.

I'm not sure about the full extent of things being broken... I'm not too worried about the selection gap, and I'm currently unable to see weather results, so I may need some help with that. I believe the calculator and unit converter are disabled by default? Unless there's a desire to enable them in the next week or so, those are probably not a reason to back this out immediately (but certainly fix in a followup). I'm not sure about the status of tab-to-search onboarding either. I suppose that might be something we already ship? So perhaps that's reason enough to back out.

(In reply to James Teow [:jteow] from comment #9)

Dao, what are you thoughts about backing out and tweaking this? I should've caught the impact to the dynamic types.

One thought I had was to combine Drew's solution of making the urlbarView-row-inner selectable by also moving the constructions of button step to inside of row-inner so that they are encapsulated when inner is selected. However, I might be missing a key reason why buttons remained separate from inner in the first place.

I think the reason is mainly the a11y tree hierarchy at this point, although I believe a11y focus would work with nested elements too. I'm not sure to what extent buttons being adjacent to urlbarView-row-inner is baked into various code and tests, but from what I understand putting buttons in urlbarView-row-inner also seems to go against what Drew was suggesting in https://phabricator.services.mozilla.com/D163630#5382438?

Replying to comment 10

I'm currently unable to see weather results, so I may need some help with that.

For the weather suggestion, I have a fix for it already. But I abandoned it because Drew was suggesting we back out your patch instead.
This should fix the weather suggestion on selection and hover:

https://phabricator.services.mozilla.com/D167453

Good call on that Dao, I should've better parsed that comment.

On the topic of potentially backing the patch out, I was under the impression based on Drew’s comment that the direction wasn’t satisfactory, so I was looking to see how to mesh his architectural changes with the existing work.

My POV is if we can spot fix the errors, we should go ahead, but I'm willing to acknowledge my understanding of the long term implications on UrlbarView is not as sophisticated as either you and Drew.

Confirming this issue as verified fixed on 112.0a1(20230223094032) and 111.0b4(20230221190142) using Windows 11, macOS 13 and Ubuntu22.