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

RESOLVED FIXED in Firefox 33



5 years ago
5 years ago


(Reporter: agibson, Assigned: Dolske)


Firefox 36
Dependency tree / graph
Bug Flags:
firefox-backlog +
qe-verify -

Firefox Tracking Flags

(firefox33 fixed, firefox34 fixed, firefox35 fixed, firefox36 fixed)



(3 attachments, 1 obsolete attachment)



5 years ago
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

5 years ago
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.
Flags: needinfo?(MattN+bmo)
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.
Flags: needinfo?(MattN+bmo)
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.
Flags: firefox-backlog?

Comment 4

5 years ago
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

5 years ago
> 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.


5 years ago
Flags: needinfo?(dolske)

Comment 6

5 years ago
Posted patch Patch v.1 (obsolete) — Splinter Review
Simple (?) workaround to just highlight the overflow button when the target is in overflow.
Assignee: nobody → dolske
Flags: needinfo?(dolske)
Attachment #8511778 - Flags: review?(MattN+bmo)
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()
Attachment #8511778 - Flags: review?(bmcbride)
Attachment #8511778 - Flags: review?(MattN+bmo)
Attachment #8511778 - Flags: review+

Comment 8

5 years ago
Posted patch Patch v.2Splinter Review
Updated with nits.
Attachment #8511778 - Attachment is obsolete: true

Comment 10

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

Comment 12

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

[Triage Comment]

Needed for 33.1 release.
Attachment #8513002 - Flags: approval-mozilla-beta+
Attachment #8513002 - Flags: approval-mozilla-aurora+

Comment 13

5 years ago
(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.
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
Iteration: --- → 36.2
Flags: qe-verify?
Flags: firefox-backlog?
Flags: firefox-backlog+


5 years ago
Duplicate of this bug: 1091556

Comment 18

5 years ago
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


5 years ago
Resolution: FIXED → ---
Since this has already been uplifted, it's easier to track this info panel positioning in another bug.
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Flags: qe-verify? → qe-verify+
QA Contact: catalin.varga
Setting qe-verify- since remaining issue is tracked separately.
Flags: qe-verify+ → qe-verify-
You need to log in before you can comment on or make changes to this bug.