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

RESOLVED FIXED in Firefox 33

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: agibson, Assigned: Dolske)

Tracking

Trunk
Firefox 36
x86
macOS
Points:
---
Dependency tree / graph
Bug Flags:
firefox-backlog +
qe-verify -

Firefox Tracking Flags

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

Details

Attachments

(3 attachments, 1 obsolete attachment)

Reporter

Description

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.
Assignee

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?
Reporter

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?
Reporter

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.
Reporter

Updated

5 years ago
Flags: needinfo?(dolske)
Assignee

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+
Assignee

Comment 8

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

Comment 10

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

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+
Assignee

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.
https://hg.mozilla.org/mozilla-central/rev/c53739a065d9
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
Iteration: --- → 36.2
Flags: qe-verify?
Flags: firefox-backlog?
Flags: firefox-backlog+
Reporter

Updated

5 years ago
Duplicate of this bug: 1091556
Reporter

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
Reporter

Updated

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