963590 - [Mac] Toolbar is not being hidden fully in full screen mode more when light weight theme is applied

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

[Tim Guan-tin Chien [:timdream] (please needinfo)]

STR:

1. Go and install https://addons.mozilla.org/en-US/firefox/addon/eco-theme/
2. Go full screen with URL given (with mozRequestFullScreen(), not Mac OS X full screen mode.
3. Inspect the top of the screen

Expected:

1. Not seeing tool bar

Actual:

1. Seeing part of the toolbar (~10px)

Note:

Only reproducible when lightweight theme is applied. Default theme does not have such problem.

Comment 1

[Matthew N. [:MattN]]

FYI: This happens on the Holly branch so it's not a regression from Australis.

To debug this, one can install the DOM Inspector extension from addons.mozilla.org and compare the CSS of the #tab-view-deck and it's children to identify where the different lies between having a theme applied and not.

Comment 2

[Kushagra Singh [:kushagra]]

Hi :) I am completely new at this and eager to contribute. Could you please guide me through this bug?

Comment 3

[Matthew N. [:MattN]]

Hi Kushagra, are you familiar with CSS and using developer tools to debug styling issues? Install DOM Inspector (mentioned in comment 1) and compare the heights and y coordinates of the descendants of #tab-view-deck.

Launch DOM Inspector from the Web Developer menu and choose File > Inspect Chrome Document > 1 … when you only have one Nightly window open. From there you can change the right pane to the "Box Model" view to see the dimensions and positions. Once you have identified differences in size or y-position, look at the "CSS Rules" view and look for selectors containing "lwtheme" (or possibly the lack of a selector which excludes lightweight themes.

Comment 4

[Kushagra Singh [:kushagra]]

i followed your instructions and found that when the lightweight theme is applied , the node deck(#tab-view-deck) and its descendents differ in y and screen y positions and height as compared to the values found in the default theme.
Moreover i didnt find any lwtheme field in the CSS rules. 
How do i now fix these issues?

Comment 5

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Matthew N. [:MattN] from comment #1)
> FYI: This happens on the Holly branch so it's not a regression from
> Australis.
> 

It's not a Australis bug. I remember seeing this since the introduction of HTML5 full screen on Firefox Mac.

(In reply to Kushagra Singh [:kush] from comment #4)
> i followed your instructions and found that when the lightweight theme is
> applied , the node deck(#tab-view-deck) and its descendents differ in y and
> screen y positions and height as compared to the values found in the default
> theme.
> Moreover i didnt find any lwtheme field in the CSS rules. 
> How do i now fix these issues?

:MattN, are you still helping out the contributor? You could filter out bugmail from mentored bugs with feature introduced in bug 869989.

Comment 6

[Matthew N. [:MattN]]

Did you find the "CSS rules" view for the right pane of DOM Inspector (DOMi)? Do you see the list of selectors in that view? You may need to hover the selector in the first columns to see them all. Note that I said the lack of selectors containing "lwtheme" could also be an issue. You can right click on a rule in that view and choose "View File" if you want to look at nearby rules. You can also just find-in-page on those stylesheets for lwtheme and see what is related.

Comment 7

[Kushagra Singh [:kushagra]]

hi Matthew, sorry for the late reply,was stuck in examinations.
i was going through the children of #tab-view-deck and checking for dimensions and lwtheme style attributes. but i noticed that only 2 children are visible in the lightweight theme fullscreen mode. should i concentrate on these 2 or continue  as before??

Comment 8

[Matthew N. [:MattN]]

No problem, there is no rush for this bug.
I'm discussing with kushagra on IRC now. I think it may not be best for a mentored bug as it requires a lot of investigation and possibly trial-and-error.

Comment 9

[Matthew N. [:MattN]]

Also take a look at https://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-fullScreen.js specifically showXULChrome and see if that is related.

Comment 10

[Tim Guan-tin Chien [:timdream] (please needinfo)]

With the mighty Browser Debugger, I can see the computed height of #navigator-toolbox shrink from 78px to 68px when the lwtheme is applied. That prevents the margin-top here from hiding the toolbar completely.

https://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-fullScreen.js#504
https://developer.mozilla.org/en-US/docs/Tools/Debugger

Still trying to isolate CSS rule that cause this.

Comment 11

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Correction: the toolbar is always 68px tall with and without lwtheme, but as soon as I go to full screen, the toolbar with default theme become 78px but the lwtheme one stays 68px.

Comment 12

[Tim Guan-tin Chien [:timdream] (please needinfo)]

I can now narrow down the difference to the following CSS rules. 

https://mxr.mozilla.org/mozilla-central/source/browser/themes/osx/browser.css#2732
https://mxr.mozilla.org/mozilla-central/source/browser/themes/osx/browser.css#3878

With looking around on MDN I realized the code have just changed in bug 941309, 18 hours ago. So I will be waiting for the new nightly tomorrow to see if bug 941309 accidentally fix the bug here.

(This is very .... exciting?)

Comment 13

[Tim Guan-tin Chien [:timdream] (please needinfo)]

I've just checked the latest Nightly and the problem is still there. 

Additionally, I spotted bug 986449 for OS X Lion full screen, but it's not a regression of bug 941309.

Comment 14

[Tim Guan-tin Chien [:timdream] (please needinfo)]

It looks like 

https://mxr.mozilla.org/mozilla-central/source/browser/themes/osx/browser.css#3880

Is the where the height is being changed. After bug 941309, the ::before block will always present but it's height is still not right.

I am trying to understand what's the requirement of OS X Lion full screen ... according to hg log it looks like the block exists to create a padding on top of the tab bar (Bug 714186 and Bug 944229). With that I still need to create a patch that does not break both bugs. It's sad that we don't have a test suite for that.

To recap, the conditions to test would be 1-a, 1-b, 2-a, 2-b, with (1) with and (2) without lwtheme, and (a) HTML5 full screen (no toolbar) and (b) OSX Lion full screen.

Comment 15

[Tim Guan-tin Chien [:timdream] (please needinfo)]

:kushagra, I am stealing this bug but if you can provide a patch with my investigation it would be great too :)

Comment 16

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: Patch v1

After bug 941309, the ::before block will always present, so we simply tweak the height regardless of lwtheme or not, for the right result in both OSX Lion full screen and HTML5 full screen.

It also makes the rules of moving background position unnecessary, fixing bug 986449 along the way.

It seems that the title bar of Australis theme is 1px short of the original one, hance the 1px tweak.

Note that the private mode window suffers almost the same bug as this one but the lines to fix is not here, so I will file another bug for that.

Comment 17

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: Patch v1.1

Update with correct diff.

Comment 18

[Matthew N. [:MattN]]

Comment on attachment 8394868 [details] [diff] [review]
Patch v1.1

Tim, this patch ended up touching the same code as bug 878436. Could you give that a try and if that patch addresses that bug fine, could you update this patch to build upon that?

Comment 19

[Tim Guan-tin Chien [:timdream] (please needinfo)]

I will work on this bug after bug 878436 lands.

Comment 20

[Matthew N. [:MattN]]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #19)
> I will work on this bug after bug 878436 lands.

FYI, this has now landed.

Comment 21

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: Patch v1.2

Please make sure my commit comment is correct too. Thanks.

Comment 22

[Matthew N. [:MattN]]

Comment on attachment 8402168 [details] [diff] [review]
Patch v1.2

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

A screenshot diff with mozscreenshots[1] showed no changes to regular fullscreen mode for the following sets on 10.9: WindowSize Toolbars TabsInTitlebar LightweightThemes CustomizeMode

The transition to leave DOM fullscreen looks nice on 10.9 but while entering the toolbars just instantly disappear after the transition. I confirmed it was like that in 2012-07 as well so this isn't a (recent) regression.

Looking back at bug 714186, I'm not really sure why these selectors included :-moz-lwtheme but I think it was just for the positioning when entering fullscreen (like bug 878436) which now works fine.

For the commit message, I would say: 
Bug 963590 - [Mac] Make sure lightweight themes don't affect fullscreen toolbar height/position.

We should uplift this to 29 (or at least 30) to avoid bitrot with other Australis titlebar/fullscreen changes since it's also simplifying and would be a nice fix with Australis.

Thanks Tim!

[1] https://github.com/mnoorenberghe/mozscreenshots

Comment 23

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: Patch for commit

Thanks Matt.

This bug is pretty annoy for me so I am happy to find out I will see it in fx29!

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/c80633c56d42

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c80633c56d42

Comment 26

[Matthew N. [:MattN]]

Comment on attachment 8403177 [details] [diff] [review]
Patch for commit

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Perhaps bug 714186 or some regression since.
User impact if declined: DOM fullscreen leaves the bottom of toolbars hanging around when a theme is applied.
Testing completed (on m-c, etc.): m-c soon and manually
Risk to taking this patch (and alternatives if risky): Low risk - just removing theme-specific part of CSS selectors.
String or IDL/UUID changes made by this patch: None

This would be nice with the other Australis tab/fullscreen fixes and reduces patch bitrot for aurora/beta.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/362dc8525345
https://hg.mozilla.org/releases/mozilla-beta/rev/2d58340206f4