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

Status: VERIFIED FIXED

(Firefox :: General, defect, P3)

Description

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Screenshot 1 - Windows 8

See screenshot.

Comment 1

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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?

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

Marked as P3 because many users on Windows use a maximized window.

Comment 3

[Matthew N. [:MattN]]

Attachment: WIP 1 - Remove wobble effect and box-shadow

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

Comment 4

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Matthew N. [:MattN] from comment #3)
> There are possible solutions and workarounds for both of these bugs

I hope so :\

Comment 5

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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 :\

Comment 6

[Matthew N. [:MattN]]

Attachment: Approach 2 v.1 - Implement <panel flip="none">

Comment 7

[Neil Deakin]

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.

Comment 8

[Matthew N. [:MattN]]

Attachment: Approach 2 v.2 - Implement <panel flip="none"> - Switch to enum

Address review comments. I'm still working on tests as I'm having a hard time figuring out where they belong.

Comment 9

[Neil Deakin]

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.

Comment 10

[Matthew N. [:MattN]]

Attachment: Tests v.1 - Sanity checks

Comment 11

[Matthew N. [:MattN]]

Thanks!

https://hg.mozilla.org/integration/fx-team/rev/0a7e5eeafed6
https://hg.mozilla.org/integration/fx-team/rev/026dc3ee0654

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 13

[Matthew N. [:MattN]]

Attachment: Tests v.1.1 - Sanity checks - Disable moveBy test on Linux

(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

Comment 14

[Matthew N. [:MattN]]

https://hg.mozilla.org/integration/fx-team/rev/e61a79610e68
https://hg.mozilla.org/integration/fx-team/rev/822ce3170f0f

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e61a79610e68
https://hg.mozilla.org/mozilla-central/rev/822ce3170f0f

Comment 16

[Catalin Varga [QA][:VarCat]]

Verified as fixed using the following environment:
FF 29.0b4
Build Id: 20140331125246
OS: Win 8 x64, Win 7 x64