1085330 - UITour: Highlight positioning breaks when icon target moves into "more tools" overflow panel.

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Alex Gibson [:agibson]]

Attachment: highlight-positioning-bug.png

When highlighting an item in the toolbar (this bug also applies to using 'showInfo' on a target), highlight & info panel positioning breaks when the browser is resized down to a width where the icon moves into the "More tools..." overflow panel.

Comment 1

[Justin Dolske [:Dolske]]

Matt: Is this a regression, or did we just never implement this?

Seems like the current code knows how to deal with opening the menupanel when items are there, so maybe this isn't too hard to fix. If not I'd also consider a wallpaper patch to just highlight the overflow chevron.

Comment 2

[Matthew N. [:MattN]]

I tried to look for an existing bug this morning but I guess there wasn't one as I couldn't find it under the fx-UITour meta bug. I thought it came up before though.

The hard part would be if we wanted to handle something moving into or out of the overflow panel while it's getting highlighted e.g. if the window resizes. It seems like we currently have a problem in that case and when we highlight something already in the overflow panel. The latter case probably isn't too hard to fix as I believe we can use the attributes that get set and the worst case is that we traverse up the tree to find the container.

The popup container is #widget-overflow and the children get the @overflowedItem attribute we can check on the widget container. There is also the @nav-bar-overflow-button attribute that we can use to know which popup to open automatically I think.

Comment 3

[Matthew N. [:MattN]]

Note that we also have code that's supposed to avoid showing an annotation if the target isn't visible and it seems like that could use some improvement too.

Comment 4

[Alex Gibson [:agibson]]

Is there any scope to get this fixed, or at least a workaround in the short-term? 

There is a risk that if we highlight something (e.g. privateWindow), and if the user has a lot of icons, it could potentially be in the overflow?

Comment 5

[Alex Gibson [:agibson]]

> If not I'd also consider a wallpaper patch to just highlight the overflow chevron.

If this would be easy enough to do, it would be better than showing a broken doorhanger / highlight icon.

Comment 6

[Justin Dolske [:Dolske]]

Attachment: Patch v.1

Simple (?) workaround to just highlight the overflow button when the target is in overflow.

Comment 7

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8511778 [details] [diff] [review]
Patch v.1

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

I know there's a short timeframe to be able to land this, so rather than do a test here and hold up landing, file a followup bug for a test?

::: browser/modules/UITour.jsm
@@ +869,5 @@
> +      if (aTarget.node.getAttribute("overflowedItem")) {
> +        let doc = aTarget.node.ownerDocument;
> +        let placement = CustomizableUI.getPlacementOfWidget(aTarget.widgetName || aTarget.node.id);
> +        let areaNode = doc.getElementById(placement.area);
> +        let chevron = doc.getElementById(areaNode.getAttribute("overflowbutton"));

Nit: areaNode.overflowable._chevron

@@ +875,5 @@
> +        highlightAnchor = chevron;
> +        targetRect = chevron.getBoundingClientRect();
> +      } else {
> +        highlightAnchor = aTarget.node;
> +        targetRect = aTarget.node.getBoundingClientRect();

Nit: Should setting targetRect out of the if/else block, and set it to highlightAnchor.getBoundingRect()

Comment 8

[Justin Dolske [:Dolske]]

Attachment: Patch v.2

Updated with nits.

Comment 9

[Justin Dolske [:Dolske]]

https://hg.mozilla.org/projects/alder/rev/48f2446e0b8b

Comment 10

[Alex Gibson [:agibson]]

Can we please also see this makes it into the gum build? Thanks!

Comment 11

[Justin Dolske [:Dolske]]

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

Comment 12

[Justin Dolske [:Dolske]]

Comment on attachment 8513002 [details] [diff] [review]
Patch v.2

[Triage Comment]

Needed for 33.1 release.

Comment 13

[Justin Dolske [:Dolske]]

(In reply to Alex Gibson [:agibson] from comment #10)
> Can we please also see this makes it into the gum build? Thanks!

This will land on Aurora shortly, and gum should pick it up with its next merge from Aurora.

Comment 14

[:Gijs (he/him)]

remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/74d96225a2a8
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/126419897a47

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/48f2446e0b8b

Comment 16

[Carsten Book [:Tomcat]]

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

Comment 18

[Alex Gibson [:agibson]]

Attachment: panel-positioning.png

Testing in Nightly this only seems to be partly fixed. Two issues remain:

1.) If the target is in the overflow panel, the highlight is correct, but the associated door-hanger is still wrongly positioned. (see attachment)
2.) Highlighting still breaks when resizing the browser

Comment 19

[Matthew N. [:MattN]]

Since this has already been uplifted, it's easier to track this info panel positioning in another bug.

Comment 20

[Matthew N. [:MattN]]

I filed bug 1091785.

Comment 21

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Setting qe-verify- since remaining issue is tracked separately.