Closed Bug 963590 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox :: Theme, defect)

All
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 31
Tracking Status
firefox28 --- wontfix
firefox29 --- fixed
firefox30 --- fixed
firefox31 --- fixed

People

(Reporter: timdream, Assigned: timdream)

References

()

Details

(Whiteboard: [mentor=MattN][lang=css][good first verify])

Attachments

(1 file, 3 obsolete files)

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.
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.
Hardware: x86 → All
Whiteboard: [mentor=MattN][lang=css]
Hi :) I am completely new at this and eager to contribute. Could you please guide me through this bug?
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.
Assignee: nobody → singh.kushagra93
Status: NEW → ASSIGNED
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?
(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.
Flags: needinfo?(MattN+bmo)
Flags: needinfo?(MattN+bmo)
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.
Flags: needinfo?(MattN+bmo)
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??
Flags: needinfo?(MattN+bmo)
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.
Flags: needinfo?(MattN+bmo)
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.
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.
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?)
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.
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.
Assignee: singh.kushagra93 → timdream
:kushagra, I am stealing this bug but if you can provide a patch with my investigation it would be great too :)
Attached file Patch v1 (obsolete) —
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.
Attachment #8394865 - Flags: review?(MattN+bmo)
Attached patch Patch v1.1 (obsolete) — Splinter Review
Update with correct diff.
Attachment #8394865 - Attachment is obsolete: true
Attachment #8394865 - Flags: review?(MattN+bmo)
Attachment #8394868 - Flags: review?(MattN+bmo)
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?
Attachment #8394868 - Flags: review?(MattN+bmo)
I will work on this bug after bug 878436 lands.
Depends on: 878436
(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.
Flags: needinfo?(timdream)
Attached patch Patch v1.2 (obsolete) — Splinter Review
Please make sure my commit comment is correct too. Thanks.
Attachment #8394868 - Attachment is obsolete: true
Attachment #8402168 - Flags: review?(MattN+bmo)
Flags: needinfo?(timdream)
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
Attachment #8402168 - Flags: review?(MattN+bmo) → review+
Attached patch Patch for commitSplinter Review
Thanks Matt.

This bug is pretty annoy for me so I am happy to find out I will see it in fx29!
Attachment #8402168 - Attachment is obsolete: true
https://hg.mozilla.org/integration/fx-team/rev/c80633c56d42
Keywords: checkin-needed
Whiteboard: [mentor=MattN][lang=css] → [mentor=MattN][lang=css][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/c80633c56d42
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [mentor=MattN][lang=css][fixed-in-fx-team] → [mentor=MattN][lang=css]
Target Milestone: --- → Firefox 31
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.
Attachment #8403177 - Flags: approval-mozilla-beta?
Attachment #8403177 - Flags: approval-mozilla-aurora?
Attachment #8403177 - Flags: approval-mozilla-beta?
Attachment #8403177 - Flags: approval-mozilla-beta+
Attachment #8403177 - Flags: approval-mozilla-aurora?
Attachment #8403177 - Flags: approval-mozilla-aurora+
Whiteboard: [mentor=MattN][lang=css] → [mentor=MattN][lang=css][good first verify]
You need to log in before you can comment on or make changes to this bug.