UITour: When highlighting app menu button, highlight is offset to the left (Implement @flip="none")

VERIFIED FIXED in Firefox 29

Status

()

defect
P3
normal
VERIFIED FIXED
6 years ago
5 years ago

People

(Reporter: Unfocused, Assigned: MattN)

Tracking

({dev-doc-needed})

unspecified
Firefox 29
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 3 obsolete attachments)

See screenshot.
Works fine if the window isn't maximized and isn't hard up against the right edge of the monitor. So, I'd say this is another issue caused by using a <xul:panel>.

Between this and bug 943759, which I believe is also caused by using <xul:panel>, I think we should consider the possibility of needing to revert back to using a <html:div>. Thoughts, Matt?
Flags: needinfo?(MattN+bmo)
Marked as P3 because many users on Windows use a maximized window.
Priority: -- → P3
This is caused by the padding for the wobble effect. I think we should remove this effect since we don't need it now. The downside is that the box-shadow is 3px and so it also relies on the padding. Getting rid of the box-shadow, padding, and border (bug 956160) gets rid of the problem and I think it workable.

An alternative is to add an attribute for panels that doesn't try to force the panel to stay entirely on-screen.

(In reply to Blair McBride [:Unfocused] from comment #1)
> Works fine if the window isn't maximized and isn't hard up against the right
> edge of the monitor. So, I'd say this is another issue caused by using a
> <xul:panel>.
> 
> Between this and bug 943759, which I believe is also caused by using
> <xul:panel>, I think we should consider the possibility of needing to revert
> back to using a <html:div>. Thoughts, Matt?

There are possible solutions and workarounds for both of these bugs which I think we should consider before switching away from a <panel> since the panel fixes z-index issues and allows the annotations to move with the target (e.g. while moving/resizing the window).
Attachment #8358521 - Flags: feedback?(bmcbride)
Flags: needinfo?(MattN+bmo)
Attachment #8358521 - Attachment description: 956162_wip1.patch → WIP 1 - Remove wobble effect and box-shadow
(In reply to Matthew N. [:MattN] from comment #3)
> There are possible solutions and workarounds for both of these bugs

I hope so :\
Comment on attachment 8358521 [details] [diff] [review]
WIP 1 - Remove wobble effect and box-shadow

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

::: browser/themes/shared/UITour.inc.css
@@ -17,5 @@
>  #UITourHighlight {
>    background-image: radial-gradient(50% 100%, rgba(0,149,220,0.4) 50%, rgba(0,149,220,0.6) 100%);
>    border-radius: 40px;
>    border: 1px solid white;
> -  box-shadow: 0 0 3px 0 rgba(0,0,0,0.5);

Removing this isn't an option, it looks awful without it :\
Attachment #8358521 - Flags: feedback?(bmcbride) → feedback-
Assignee: nobody → MattN+bmo
Attachment #8358521 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8359827 - Flags: review?(enndeakin)

Comment 7

6 years ago
Comment on attachment 8359827 [details] [diff] [review]
Approach 2 v.1 - Implement <panel flip="none">

>   // If a panel is being moved, don't constrain or flip it. But always do this for
>   // content shells, so that the popup doesn't extend outside the containing frame.
>-  if (mInContentShell || !aIsMove || mPopupType != ePopupTypePanel) {
>+  if (!mFlipNone && (mInContentShell || !aIsMove || mPopupType != ePopupTypePanel)) {
>     nsRect screenRect = GetConstraintRect(anchorRect, rootScreenRect);

We have to flip/constrain when mInContentShell is true as this will be a non-chrome window.


>   // One of nsIPopupBoxObject::ROLLUP_DEFAULT/ROLLUP_CONSUME/ROLLUP_NO_CONSUME
>   int8_t mConsumeRollupEvent;
>+  bool mFlipNone; // never flip or resize
>   bool mFlipBoth; // flip in both directions
>   bool mSlide; // allow the arrow to "slide" instead of resizing
> 

We should combine all of these three into one field of an enum type as they are mutally exclusive.
Address review comments. I'm still working on tests as I'm having a hard time figuring out where they belong.
Attachment #8359827 - Attachment is obsolete: true
Attachment #8359827 - Flags: review?(enndeakin)
Attachment #8362997 - Flags: review?(enndeakin)

Comment 9

6 years ago
Comment on attachment 8362997 [details] [diff] [review]
Approach 2 v.2 - Implement <panel flip="none"> - Switch to enum

>   // One of nsIPopupBoxObject::ROLLUP_DEFAULT/ROLLUP_CONSUME/ROLLUP_NO_CONSUME
>   int8_t mConsumeRollupEvent;
>-  bool mFlipBoth; // flip in both directions
>-  bool mSlide; // allow the arrow to "slide" instead of resizing

Just copy these comments over to the enum lines and this is ok.
Attachment #8362997 - Flags: review?(enndeakin) → review+
Keywords: dev-doc-needed
Summary: UITour: When highlighting app menu button, highlight is offset to the left → UITour: When highlighting app menu button, highlight is offset to the left (Implement @flip="none")

Updated

6 years ago
Attachment #8363829 - Flags: review?(enndeakin) → review+
(In reply to Wes Kocher (:KWierso) from comment #12)
> Backed both out in
> https://hg.mozilla.org/integration/fx-team/rev/62adaec0470b for m-oth
> failures like:
> https://tbpl.mozilla.org/php/getParsedLog.php?id=33415256&tree=Fx-Team

This was a Linux-only failure and Enn pointed out on IRC that "there's a weird issue on linux where windows are moved yet the position reported by the system doesn't get updated at the same time, so a number of tests just skip window moving on linux". I've done the same here.

Try push: https://tbpl.mozilla.org/?tree=Try&rev=2d9dc27abf06
Attachment #8363829 - Attachment is obsolete: true
Attachment #8364434 - Flags: review?(enndeakin)

Updated

6 years ago
Attachment #8364434 - Flags: review?(enndeakin) → review+
https://hg.mozilla.org/mozilla-central/rev/e61a79610e68
https://hg.mozilla.org/mozilla-central/rev/822ce3170f0f
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 29
Verified as fixed using the following environment:
FF 29.0b4
Build Id: 20140331125246
OS: Win 8 x64, Win 7 x64
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.