Closed Bug 1085330 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox :: General, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 36
Iteration:
36.2
Tracking Status
firefox33 --- fixed
firefox34 --- fixed
firefox35 --- fixed
firefox36 --- fixed

People

(Reporter: agibson, Assigned: Dolske)

References

Details

Attachments

(3 files, 1 obsolete file)

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.
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?
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?
> 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.
Flags: needinfo?(dolske)
Attached 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+
Attached patch Patch v.2Splinter Review
Updated with nits.
Attachment #8511778 - Attachment is obsolete: true
Can we please also see this makes it into the gum build? Thanks!
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+
(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.
https://hg.mozilla.org/mozilla-central/rev/c53739a065d9
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
Iteration: --- → 36.2
Flags: qe-verify?
Flags: firefox-backlog?
Flags: firefox-backlog+
Duplicate of this bug: 1091556
Attached image 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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Since this has already been uplifted, it's easier to track this info panel positioning in another bug.
Status: REOPENED → RESOLVED
Closed: 6 years ago6 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.