Closed Bug 1790861 Opened 2 years ago Closed 5 months ago

Firefox View's Last Active tile includes the "Last Active" text at the start of its suggested google search context-menu-item

Categories

(Firefox :: Firefox View, defect, P3)

defect

Tracking

()

RESOLVED WONTFIX

People

(Reporter: dholbert, Unassigned)

References

(Blocks 1 open bug)

Details

(Whiteboard: [fidefe-firefox-view] )

Attachments

(3 files, 1 obsolete file)

STR:

  1. Open Firefox View (with Sync configured so that you have a "Last Active" tile)
  2. Right-click your "Last Active" tile and look at the context menu.

ACTUAL RESULTS:
The "Search" context-menu suggestion includes the "Last Active" text. E.g. in my case my last-active tab is Yahoo, and the context menu search suggestion is Search Google for Last Active Yah...

EXPECTED RESULTS:
"Last Active" shouldn't be part of the search term there. (That's part of our UI rather than part of the content.)
Or: we shouldn't offer this "search" contextmenu item.

(It's not clear that "Search Google for [whatever]" is a particularly useful operation but it seems to be something we offer for all links using the link text.)

Attached image screenshot
Whiteboard: [fidefe-firefox-view]
Severity: -- → S3
Priority: -- → P3
Blocks: 1797520
No longer blocks: firefox-view
Assignee: nobody → prathikshaprasadsuman
Status: NEW → ASSIGNED
Attachment #9312863 - Attachment description: WIP: Bug 1790861 - Fix search string rendering in Firefox View's context menu for synced tab links. → Bug 1790861 - Fix search string rendering in Firefox View's context menu for synced tab links.

It actually looks like the other UI components here end up being included in the suggested search here (the site's domain name, the device name and the timestamp). They're at the end, so they're not visible in the context menu, but if you actually click through and perform the search, you'll end up with them included.

We should probably strip those out as well.

Here's a screencast showing the actual search query that we submit if you happen to use this context-menu option here. Towards the end of the video, I highlight all of the parts of the query that come from "annotations"/labels which I wouldn't expect to be included in the search.

(In reply to Daniel Holbert [:dholbert] from comment #4)

Created attachment 9312903 [details]
screencast showing what happens when you actually click through to perform the search as of Jan 2023

Here's a screencast showing the actual search query that we submit if you happen to use this context-menu option here. Towards the end of the video, I highlight all of the parts of the query that come from "annotations"/labels which I wouldn't expect to be included in the search.

So fwiw this is because the entire link's text content is used as the search text. This is just a generic context menu item, it's not specific to Firefox View.

We want the entire card to be a link because that provides a better experience for accessibility users.

There isn't really a good mechanism to exclude text from the link - other than using pseudo-text, because that doesn't get picked up when looking for textContent or similar on DOM nodes. But it feels pretty yucky to force all these things into pseudo-text.

Do you have a suggestion for a better mechanism/fix here?

Flags: needinfo?(dholbert)

I don't have a great suggestion, no. And I think it's fine for this to produce a slightly-strange search query (as it already does), since honestly this context-menu item probably isn't used super-often, and we do kinda-similar-things if you right-click the text on an about:home pocket suggested-story tile.

There are two concrete things that I think we should improve here if we can though:
(1) It's weird that "Last Active" is the prominent thing that we show in the context menu (that's what seemed odd to me at first). Maybe we can reorder the DOM such that it comes after the story title, so that it's not the first words (and probably just gets ellipsized away) in the context menu search-suggestion?

(2) It's unfortunate that we're non-obviously including the device name ("dholbert's Nightly on violet") in the suggested-search-text there. That information (Firefox Account username, device hostname) is potentially mildly personal, so it's not great that we're stuffing that into a suggested Google Search, which users might casually perform out of curiosity, without realizing that that information is included. If we can at least exclude that part from the link text (to exclude it from being part of the suggested-search), I think that'd be worth doing.

Flags: needinfo?(dholbert)

One prior-art example where we seem to "do this right" (and hence might have some ideas to be cribbed from) is the Theme section on about:addons.

There, each theme gets a tile, and the whole tile is clickable, but most of the area doesn't pop up this suggested-search text (which is ~fine, it's a bit odd that we're suggesting it in the first place) -- only the theme title (the thing that looks most like a link) has a suggested search, with the search just being the theme title itself.

(In reply to Daniel Holbert [:dholbert] from comment #7)

One prior-art example where we seem to "do this right" (and hence might have some ideas to be cribbed from) is the Theme section on about:addons.

The difference is that those aren't links. The bit that is a link (the theme name) is kind of confusing because the link targets don't actually work (ie "open link in new tab" opens a "The address wasn’t understood" tab, which is probably a bug).

Yeah, I figured those about:addons cards were getting around this via some form of hackery. I realize there are tradeoffs here and I'm not claiming either approach is superior on-the-whole, but I do think the two concrete points from comment 6 are both worth addressing if possible.

(In reply to :Gijs (he/him) from comment #5)

There isn't really a good mechanism to exclude text from the link - other than using pseudo-text, because that doesn't get picked up when looking for textContent or similar on DOM nodes. But it feels pretty yucky to force all these things into pseudo-text.

Gijs, when you say "force all these things into pseudo-text", what does this mean? Is there not a way to only do this for the "Search Google for.." portion of the menu?

If not, I also agree with Mark and think that it's acceptable to keep the menu as is while trying to address the concerns around reordering the text sequence. If we have the ability to remove text, I'd think we'd try and remove both the device name and the "Last Active" designation, as neither are useful in the actual Search.

Summary: Firefox View's Last Active tile includes the "Last Active" text in its suggested google search context-menu-item → Firefox View's Last Active tile includes the "Last Active" text at the start of its suggested google search context-menu-item

Adding a little counterpoint-nuance to one of my points in comment 6:

(1) It's weird that "Last Active" is the prominent thing that we show in the context menu (that's what seemed odd to me at first). Maybe we can reorder the DOM such that it comes after the story title, so that it's not the first words (and probably just gets ellipsized away) in the context menu search-suggestion?

Counterpoint: maybe the "Last Active" badge's current early-DOM-ordering (before the article title) is intentional because maybe that's truly what we want a screen reader to read as the very first thing in this section. If that's the case, then it's not worth worrying about reordering the DOM just for the benefit of this rarely-used context-menu-item.

Yeah, that is a good point and I was wondering how this will affect screen reader users (which I'm sure we all agree should be the priority). If this is actually a rarely used context-menu-item it begs the question of whether its worth the engineering effort to fix since there isn't a good solution.

Flags: needinfo?(mreschenberg)
Flags: needinfo?(ayeddi)

(In reply to Daniel Holbert [:dholbert] from comment #11)

Adding a little counterpoint-nuance to one of my points in comment 6:

(1) It's weird that "Last Active" is the prominent thing that we show in the context menu (that's what seemed odd to me at first). Maybe we can reorder the DOM such that it comes after the story title, so that it's not the first words (and probably just gets ellipsized away) in the context menu search-suggestion?

Counterpoint: maybe the "Last Active" badge's current early-DOM-ordering (before the article title) is intentional because maybe that's truly what we want a screen reader to read as the very first thing in this section. If that's the case, then it's not worth worrying about reordering the DOM just for the benefit of this rarely-used context-menu-item.

I agree that the Last Active is something we may want to keep at the beginning of the link's accessible name, because it is a unique description of the link, plus, it's shorter than average link titles, so it does not let a user to wait long until the website's title is announced too. Also, for speech-to-text users (like Dragon NaturallySpeaking on Windows and Voice Control on Mac) it is easy to call the link by its unique identificator (Last Active) to activate rather than trying to call, for instance, a Gmail and then call a number of the link, if there are a few Gmails that are in the list. Or Bugzillas. Or else.

That's being said, maybe we can create a system-wide solution for a menu item text so the information from the link textContent can be customized or at least trimmed from a user name and other unnecessary information going forward? It looks like this is not the first instance and it probably won't be the last one. I am very suspicious of a pseudo-text, because it generally is not exposed properly to assistive technology and would create a need to do a workaround to provide a meaningful accessible name to these tile links. While aria-label or aria-labelledby could be a solution to some extent, but the first rule of ARIA applies here too - if we can avoid using it in a favor of a semantic HTML we should.

Flags: needinfo?(mreschenberg)
Flags: needinfo?(ayeddi)

Closing this one as the solution for consideration would go beyond the scope of FxView.

Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → WONTFIX

OK, I'll spin off a new bug for hiding/removing the device-ID text, since there seems to be consensus on that part being not useful & to some extent actively-bad, to have buried at the end of the suggested search.

Thanks Daniel!

Blocks: 1812107

I don't think we should close this right now. Gijs had a good suggestion (he left it in Pratiksha's patch) which would fix it at a higher level but would likely fix this issue without having to do the psuedo-content approach. I think we should consider that option first before closing it out. I'm sure Ray didn't see that before closing.

Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---

Apologies for jumping the gun here. I mistakenly overlooked Gijs' suggestion.

Linking Gijs' suggestion here (https://phabricator.services.mozilla.com/D167166#5516182) for continuation purposes for anyone picking up this ticket in the future. I'm unassigning myself for now because of my move to the Fx Client team. Please feel free to reach out to me on Slack if I can provide any context or support in resolving this bug.

Assignee: prathikshaprasadsuman → nobody
Attachment #9312863 - Attachment is obsolete: true
Assignee: nobody → sfoster

I may come back to this but not currently working on it.

Assignee: sfoster → nobody

This fix is no longer needed as the new Firefox View has gone live, and we don't display any "Last active" badge. While I do see some extraneous info at the end the search text with the current links in Firefox View, I think we can address those in bug 1812107. Closing this one out.

Status: REOPENED → RESOLVED
Closed: 1 year ago5 months ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: