1814630 - Can't arrow down past the QuickAction row when that row only has disabled buttons

Status: RESOLVED DUPLICATE of bug 1790635

(Firefox :: Address Bar, defect, P1)

Description

[Dão Gottwald [:dao]]

STR:

• Open a new tab
• Type "screen" in the address bar
• Try to arrow down past the disabled "Take a screenshot" button.

This is likely a regression from bug 1801298.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1801298

Comment 2

[Dão Gottwald [:dao]]

Chatted about this with Dale last Friday:

[dao] hmm, https://bugzilla.mozilla.org/show_bug.cgi?id=1814630 is going to be messy... can we make it so that we don't have rows with all-disabled elements? @Dale? :|
[dale] Argh sorry about to run out, will take a deeper look tomorrow night.We have been back and forward on this, this issue is it can be confusing to hide elements that cant be used in a specific context, people were looking for screenshot and confused why it was not visible whereas when its disabled it is is a little clearer whats happening.
[dale] It is more important when the user has entered actions mode though vs standard entry mode
[dao] apparently https://bugzilla.mozilla.org/show_bug.cgi?id=1789727 would fix this but seems to be moving slowly
[dale] Yeh https://bugzilla.mozilla.org/show_bug.cgi?id=1805774 fixed some stuff related to that
[dao] Here's where this fails btw:

https://searchfox.org/mozilla-central/rev/2d820afafbe62b0148e2766a4d371556edefe36f/browser/components/urlbar/UrlbarView.sys.mjs#316-319

https://searchfox.org/mozilla-central/rev/2d820afafbe62b0148e2766a4d371556edefe36f/browser/components/urlbar/UrlbarView.sys.mjs#171-173

Removing the marked lines in the selectedRowIndex setter would fix the bug for arrow down (letting it move to the next row instead of clearing the selection), but arrow up still gets stuck... this is where we enter the messy territory

[dale] Hrm, that code seemed familiar, not 100% but I think that is the situation I was trying to fix in https://phabricator.services.mozilla.com/D152223
[dale] It seems like that code that deals with UNSELECTABLE_ELEMENT_SELECTOR is gone
[dao] yeah, things have changed quite a bit. especially with our new arrow behavior we wouldn't even use that code path anymore
[dao] arrow keys now move between rows rather than selectable elements where we wouldn't care if the next selectable element would be in the same row or the next one or the the one after that or whatever

Dale, do you have further thoughts on how we should approach this?

Comment 3

[Dale Harvey (:daleharvey)]

This happens with QuickActions but it could conceivably happen with other UrlbarProviders if they were to show some type of notice without anything actionable contained (I dont think there are any at the moment, but it seems like something that could feasibly happen).

Do you foresee any issues with reintroducing a check before selecting a row to check whether the row is a dynamicResult with no selectable buttons and if so skipping selection of that row?

That seems the ideal solution, but if you see technical / ux problems with that then I think this will have to go to UX, not having quickactions return any items in that case is doable but I would be worried the inconsistent behaviour would be confusing

Comment 4

[Donal Meehan [:dmeehan]]

:dao this is the final week of 111 nightly, 111 goes to beta next week.
Could this be triaged?

Comment 5

[Dão Gottwald [:dao]]

(In reply to Dale Harvey (:daleharvey) from comment #3)

This happens with QuickActions but it could conceivably happen with other UrlbarProviders if they were to show some type of notice without anything actionable contained (I dont think there are any at the moment, but it seems like something that could feasibly happen).

I don't know, an unselectable row seems like an odd concept for what is essentially still an autocomplete UI. I think if we want to add notices we should have an API for that rather than mixing this in with results.

Do you foresee any issues with reintroducing a check before selecting a row to check whether the row is a dynamicResult with no selectable buttons and if so skipping selection of that row?

Well yes, the code I linked to isn't prepared for that because it deals with rows rather than selectable elements. I'm sure something could be hacked together, but it wasn't obvious to me when looking into this and the result may be messy. So given my skepticism about the concept of unselectable rows, I remain reluctant to go down that path.

That seems the ideal solution, but if you see technical / ux problems with that then I think this will have to go to UX, not having quickactions return any items in that case is doable but I would be worried the inconsistent behaviour would be confusing

It seems confusing either way. I certainly didn't understand why I was offered the disabled screenshot action.

(In reply to Donal Meehan [:dmeehan] from comment #4)

:dao this is the final week of 111 nightly, 111 goes to beta next week.
Could this be triaged?

QuickActions are only enabled in Nightly and Early Beta, so I think we're okay for now. Dale, is there a timeline for letting QuickActions ride a release train?

Comment 6

[Dale Harvey (:daleharvey)]

We dont have any plans to enable on release and will keep this bug in mind, I just remembered that Marco did file and we have a patch to disable actions so I will check with UX but it may be that we want to land it

https://bugzilla.mozilla.org/show_bug.cgi?id=1790635

Comment 7

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:adw, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 8

[Dale Harvey (:daleharvey)]

Dao said he does not think it would be a great idea to have unselectable rows in the url given the new behaviour of keyboard navigation so the best solution here is probably the previously submitted patch