Closed Bug 885052 Opened 11 years ago Closed 10 years ago

Fullscreen-button can has pressed state in customization mode

Categories

(Firefox :: Toolbars and Customization, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 29

People

(Reporter: jaws, Assigned: jaws)

References

(Blocks 1 open bug)

Details

(Whiteboard: [Australis:P4])

Attachments

(1 file)

      No description provided.
Whiteboard: [Australis:M?] → [Australis:M?][Australis:P4]
To fix this bug, you'll need to clone the repository at https://hg.mozilla.org/projects/ux/.

We'll probably want to add to the selector that applies the pressed-in/active state so that it isn't applied when #main-window[customizing] is true.
Whiteboard: [Australis:M?][Australis:P4] → [Australis:M?][Australis:P4][good-first-bug][mentor=jaws][lang=css]
Whiteboard: [Australis:M?][Australis:P4][good-first-bug][mentor=jaws][lang=css] → [Australis:M?][Australis:P4][good first bug][mentor=jaws][lang=css]
STR:
1) Open a build of Firefox running Australis.
2) Click on the menu button and then click on the Full Screen button
3) Move your mouse to the top of the screen to see the Navigation Toolbar reappear
4) Click on the menu button
5) Notice that the Full Screen button is shown as active, since you are in full screen.
6) Click on Customize
7) Notice that the Full Screen button is still shown as active.

We don't want buttons in customize mode to resemble current state of the browser, as this mode should feel detached from what else is going on in the browser at the moment.
Hey I am interested in this bug. 
How can I help ?
I am new here !
Hi Akshat, I have provided some steps for you in comment #1. Please let me know if I should explain more.
Assignee: nobody → aksht.kedia
Status: NEW → ASSIGNED
Any update on this? Let me know if there is anything I can do to help.
Flags: needinfo?(aksht.kedia)
Can you give me a bit more of information about the bug ? I do get what exactly is happening but not really understand what I have to implement.
Flags: needinfo?(aksht.kedia)
(In reply to Akshat Kedia from comment #6)
> Can you give me a bit more of information about the bug ? I do get what
> exactly is happening but not really understand what I have to implement.

When customization mode is entered, we should stop displaying the full screen button as though it is active/enabled.

We will probably need to adjust some CSS that checks for [checked="true"] so that it doesn't apply if #main-window[customizing] is present.

For example, from http://mxr.mozilla.org/mozilla-central/source/browser/themes/osx/browser.css#1032 we could do the following:

>  #fullscreen-button {
>    -moz-image-region: rect(0, 340px, 20px, 320px);
>  }
> 
> -#fullscreen-button[checked="true"],
> +#main-window[customizing] #fullscreen-button[checked="true"],
>  #restore-button {
>    -moz-image-region: rect(0, 360px, 20px, 340px);
>  }

I think the Windows style is implemented at http://mxr.mozilla.org/mozilla-central/source/browser/themes/windows/browser.css#893 but that is a much more generic rule and might not be so easy to work around. A hacky way to do it would be the following:

>  @navbarLargeIcons@ .toolbarbutton-1 > .toolbarbutton-menubutton-button:not([disabled]):hover:active > .toolbarbutton-icon,
>  @navbarLargeIcons@ .toolbarbutton-1[open] > .toolbarbutton-menubutton-dropmarker:not([disabled]) > .dropmarker-icon,
> -@navbarLargeIcons@ .toolbarbutton-1:not([disabled]):-moz-any([open],[checked],:hover:active) > .toolbarbutton-icon,
> +@navbarLargeIcons@ .toolbarbutton-1:not([disabled]):not(#fullscreen-button):-moz-any([open],[checked],:hover:active) > .toolbarbutton-icon,
> +#main-window:not([customizing]) @navbarLargeIcons@ #fullscreen-button:not([disabled]):-moz-any([open],[checked],:hover:active) > .toolbarbutton-icon,
>  @navbarLargeIcons@ .toolbarbutton-1:not([disabled]):-moz-any([open],[checked],:hover:active) > .toolbarbutton-badge-container {
>   background-image: linear-gradient(hsla(0,0%,100%,.6), hsla(0,0%,100%,.1));
>    background-color: hsla(210,54%,20%,.15);
>    border-color: hsla(210,54%,20%,.3) hsla(210,54%,20%,.35) hsla(210,54%,20%,.4);
>    box-shadow: 0 1px 1px hsla(210,54%,20%,.1) inset,
>                0 0 1px hsla(210,54%,20%,.2) inset,
>                /* allows windows-keyhole-forward-clip-path to be used for non-hover as well as hover: */
>                0 1px 0 hsla(210,54%,20%,0),
>                0 0 2px hsla(210,54%,20%,0);
>    text-shadow: none;
>    transition: none;
>  }

Although IMO it's kinda ugly, off the top of my head I'm not sure of a better solution for Windows. I hope that helps.
Can you please tell me what does this CSS exactly do ?
 
#fullscreen-button[customizableui-areatype="toolbar"] {
  list-style-image: url("moz-icon://stock/gtk-fullscreen?size=toolbar");
}
(In reply to Akshat Kedia from comment #8)
> Can you please tell me what does this CSS exactly do ?
>  
> #fullscreen-button[customizableui-areatype="toolbar"] {
>   list-style-image: url("moz-icon://stock/gtk-fullscreen?size=toolbar");
> }

It sets the icon for the fullscreen button to url("moz-icon://stock/gtk-fullscreen?size=toolbar") when the button is in the toolbar. That url("moz-icon...") thing points to the stock GTK fullscreen icon. Does that help?
Any updates on this bug, Akshat? Are you still working on it?
Flags: needinfo?(aksht.kedia)
I am working on a linux environment. Can you please help me with the implementation here ?
Flags: needinfo?(aksht.kedia) → needinfo?(jaws)
Talked over IRC.
Flags: needinfo?(jaws)
I tried the above windows implementation but it does not seem to fix the bug(I used a windows machine). I am still able to reproduce the bug.
Flags: needinfo?(jaws)
Attached patch PatchSplinter Review
I'm sorry Akshat, the bug turned out to be quite different than I was expecting. I've put up a patch here that fixes the issue and includes a test that reproduces the issue and ensures that it won't break in the future. Please let me know what you think of it.
Assignee: aksht.kedia → jaws
Attachment #8348461 - Flags: review?(bmcbride)
Attachment #8348461 - Flags: feedback?(aksht.kedia)
Flags: needinfo?(jaws)
Whiteboard: [Australis:M?][Australis:P4][good first bug][mentor=jaws][lang=css] → [Australis:P4]
Attachment #8348461 - Flags: review?(bmcbride) → review+
(In reply to Jared Wein [:jaws] (Away 20 Dec to 2 Jan) from comment #17)
> https://hg.mozilla.org/integration/fx-team/rev/af77b8d3a9e1

hey Jared, had to backout this again for testfailures like https://tbpl.mozilla.org/php/getParsedLog.php?id=32074966&tree=Fx-Team
Made some tweaks to the test and confirmed that it would work on Linux. Repushed,
https://hg.mozilla.org/integration/fx-team/rev/98aa3895f04d
https://hg.mozilla.org/mozilla-central/rev/98aa3895f04d
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 29
Comment on attachment 8348461 [details] [diff] [review]
Patch

I had sort of started getting an idea that we were looking at the wrong place. Great this bug is fixed now :D Although, I didnt really understand how you fixed it.
Attachment #8348461 - Flags: feedback?(aksht.kedia) → feedback+
This test is still causing frequent random orange, see bug 951403. :-(
Depends on: 951403
(In reply to :Gijs Kruitbosch (Unavailable Dec 19 - Jan 2) from comment #22)
> This test is still causing frequent random orange, see bug 951403. :-(

The patch in bug 933926 includes the potential fix for this intermittent orange. I'll see about separating it out so it can land sooner.
You need to log in before you can comment on or make changes to this bug.