937415 - UITour: Add ability to highlight the selected tab's icon

Status: RESOLVED FIXED

(Firefox :: General, defect, P1)

Description

[Alex Gibson [:agibson]]

We would like to add the ability to highlight the current active tab, in a similar fashion to how icon highlighting works.

Comment 1

[Matthew N. [:MattN]]

The plan from yesterday was to highlight the curve on the start of the selected tab rather than the whole tab.

How about a target name of "selectedtabstart"?

By the way, why are the target names all lowercase instead of camel case or with hyphens?

Comment 2

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Matthew N. [:MattN] (catching up on reviews) from comment #1)
> The plan from yesterday was to highlight the curve on the start of the
> selected tab rather than the whole tab.

Awaiting mockup.
 
> By the way, why are the target names all lowercase instead of camel case or
> with hyphens?

No reason. Original patch had them as ALLCAPS, which was questioned in review, so I changed to nocaps. If you think its worth changing to CamelCase or hythenated-names, we can.

Comment 3

[Matthew N. [:MattN]]

Attachment: [e-mail]

Here is the mockup.

Comment 4

[Matthew N. [:MattN]]

Attachment: WIP 1 - Try first available of tab-icon-image, tab-throbber, tab-background-start

The main problem with switching to highlighting the tab icon, instead of the curve, is that the icon isn't always present:
* It is hidden when the tab throbber is shown
* It is hidden (since Australis) when the page doesn't provide an icon

The visibility can change while the annotation is open and that can cause the annotation to no longer be anchored properly (and jump to anchoring at the corner of the window). Shall I add a mutation observer on the selected tab to watch for @image and @busy attribute changes on the selected tab to recalculate which of the three descendant elements to use? Or shall we see about going back to .tab-background-start only?

Comment 5

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8346415 [details] [diff] [review]
WIP 1 - Try first available of tab-icon-image, tab-throbber, tab-background-start

Review of attachment 8346415 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Matthew N. [:MattN] from comment #4)
> Shall I add a mutation observer on the selected
> tab to watch for @image and @busy attribute changes on the selected tab to
> recalculate which of the three descendant elements to use? Or shall we see
> about going back to .tab-background-start only?

Hm, yea. Using just .tab-background-start would be a *lot* easier - the only difference is the offset we'd use to get the same effect.

Or am I missing some downside?

Comment 6

[Matthew N. [:MattN]]

(In reply to Blair McBride [:Unfocused] from comment #5)
> (In reply to Matthew N. [:MattN] from comment #4)
> > Shall I add a mutation observer on the selected
> > tab to watch for @image and @busy attribute changes on the selected tab to
> > recalculate which of the three descendant elements to use? Or shall we see
> > about going back to .tab-background-start only?
> 
> Hm, yea. Using just .tab-background-start would be a *lot* easier - the only
> difference is the offset we'd use to get the same effect.
> 
> Or am I missing some downside?

I didn't really want to add a special offset for certain targets so I was hoping we could just center on the tab curve element and adjust the size if necessary but not be perfectly centered.

If you're talking about anchoring on the .tab-background-start and then offsetting from the anchor to center on the icon/throbber then it's still going to look a bit weird when there is neither. Perhaps we can ignore that case though since I suspect mozilla.org will always have a favicon.

Comment 7

[Matthew N. [:MattN]]

Attachment: Example of centering on .tab-background-start

Comment 8

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Matthew N. [:MattN] from comment #6)
> If you're talking about anchoring on the .tab-background-start and then
> offsetting from the anchor to center on the icon/throbber then it's still
> going to look a bit weird when there is neither. 

IMO, still better than what looks like highlighting primarily the inactive tab (which, subjectively, is what attachment 8347080 [details] looks like to me). 

> Perhaps we can ignore that
> case though since I suspect mozilla.org will always have a favicon.

Yea, I think we're spending too much time on this detail, given that.

Comment 9

[Matthew N. [:MattN]]

Attachment: v.1 Use tab-icon-image with an anonid

This just uses tab-icon-image with an anonid to avoid it from breaking when extensions change the class attribute.

Comment 10

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8358447 [details] [diff] [review]
v.1 Use tab-icon-image with an anonid

Review of attachment 8358447 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with a couple of fixups.

::: browser/modules/UITour.jsm
@@ +63,5 @@
>                                                          "searchbar-engine-button");
>        },
>        widgetName: "search-container",
>      }],
> +    ["selectedTabIcon", {

Hm, can you also add "selectedTabCurve", which just calls the handler for "selectedTabIcon"?

Reasoning: This is centering on the tab icon, the page is wanting the center on the tab curve but the icon is close enough. Both of those are fine. But if we were to eventually add a highlight just for the tab curve, it wouldn't be named "selectedTabIcon"... nor would we ideally want to need to update the page.

@@ +65,5 @@
>        widgetName: "search-container",
>      }],
> +    ["selectedTabIcon", {
> +      query: (aDocument) => {
> +        let selectedtab = aDocument.querySelector(".tabbrowser-tab[selected='true']");

Querying on an arbitrary attribute like this would be very slow, compared to just using
  aDocument.defaultView.gBrowser.selectedTab

Comment 11

[Matthew N. [:MattN]]

(In reply to Blair McBride [:Unfocused] from comment #10)
> Comment on attachment 8358447 [details] [diff] [review]
> v.1 Use tab-icon-image with an anonid
> 
> Hm, can you also add "selectedTabCurve", which just calls the handler for
> "selectedTabIcon"?

I believe it would require moving the function outside the Map in order to make the alias which is not ideal.

> Reasoning: This is centering on the tab icon, the page is wanting the center
> on the tab curve but the icon is close enough. 

Michael wanted the highlight to be centered on the icon so I don't think we should add selectedTabCurve as an alias. I believe Michael said all highlights should be centered on a glyph.

Comment 12

[Matthew N. [:MattN]]

https://hg.mozilla.org/integration/fx-team/rev/c1c72268dba6

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c1c72268dba6