Closed Bug 827938 Opened 7 years ago Closed 7 years ago

Appear the non-expected line on browser toolbar on OSX when tab-on-bottom.

Categories

(Firefox :: Theme, defect)

x86
macOS
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 21
Tracking Status
firefox20 --- verified

People

(Reporter: tetsuharu, Assigned: Ehsan)

References

Details

(Keywords: regression)

Attachments

(3 files)

Attached image screenshot
[Environment]
* Nightly 21, http://hg.mozilla.org/mozilla-central/rev/dccab70d8554
* OSX 10.8

[Result]
Please see screenshot.
Blocks: 749394
Keywords: regression
Dao, should I look into this, or are we removing tabs-on-bottom?
It's unclear if and when we'd remove tabs on bottom; we clearly aren't doing it at this time.
Assignee: nobody → ehsan
Attached patch Patch (v1)Splinter Review
Attachment #699841 - Flags: review?(dao)
Comment on attachment 699841 [details] [diff] [review]
Patch (v1)

It's not clear to me what's going on here and why the combination of privatebrowsingmode=temporary and tabsontop=false requires the nav bar to use the same styling as when a lightweight theme is applied. Can you explain?
(In reply to comment #4)
> Comment on attachment 699841 [details] [diff] [review]
>   --> https://bugzilla.mozilla.org/attachment.cgi?id=699841
> Patch (v1)
> 
> It's not clear to me what's going on here and why the combination of
> privatebrowsingmode=temporary and tabsontop=false requires the nav bar to use
> the same styling as when a lightweight theme is applied. Can you explain?

We use drawintitlebar on Mac for private windows, which requires us to use a bunch of :-moz-lwtheme for private windows as well.  We need to disable the nav-bar appearance to make sure that the extra one pixel line doesn't leak through, the same way that we need to do that with a light-weight theme.

Technically, we should probably use the same rule for both tabs on top and bottom, but the problem doesn't occur in the tabs on top mode, and I didn't want this fix to do more than the minimum necessary to fix the problem.
So wouldn't it be preferable to use the drawintitlebar attribute for the selector?
(In reply to comment #6)
> So wouldn't it be preferable to use the drawintitlebar attribute for the
> selector?

Do you mean something like this?

 :root[drawintitlebar=true] #PersonalToolbar,
 :root[drawintitlebar=true] #nav-bar,
 [privatebrowsingmode=temporary] #nav-bar[tabsontop=false]

I can do that if you want.  The reason I did not do that is that this attribute is never used inside a selector in our theme code, as far as I can tell.
(In reply to :Ehsan Akhgari from comment #7)
> (In reply to comment #6)
> > So wouldn't it be preferable to use the drawintitlebar attribute for the
> > selector?
> 
> Do you mean something like this?
> 
>  :root[drawintitlebar=true] #PersonalToolbar,

Would we want this? Your current patch doesn't touch #PersonalToolbar.

>  :root[drawintitlebar=true] #nav-bar,
>  [privatebrowsingmode=temporary] #nav-bar[tabsontop=false]

The last selector would already be covered by the previous one...
(In reply to comment #8)
> (In reply to :Ehsan Akhgari from comment #7)
> > (In reply to comment #6)
> > > So wouldn't it be preferable to use the drawintitlebar attribute for the
> > > selector?
> > 
> > Do you mean something like this?
> > 
> >  :root[drawintitlebar=true] #PersonalToolbar,
> 
> Would we want this? Your current patch doesn't touch #PersonalToolbar.

Hmm, probably not! :-)

> >  :root[drawintitlebar=true] #nav-bar,
> >  [privatebrowsingmode=temporary] #nav-bar[tabsontop=false]
> 
> The last selector would already be covered by the previous one...

I see where you're going with this now.  OK, let me test this quickly and submit a new patch!
(In reply to :Ehsan Akhgari from comment #9)
> > >  :root[drawintitlebar=true] #nav-bar,
> > >  [privatebrowsingmode=temporary] #nav-bar[tabsontop=false]
> > 
> > The last selector would already be covered by the previous one...
> 
> I see where you're going with this now.  OK, let me test this quickly and
> submit a new patch!

Hmm, no we can't drop the third selector this way, since in that case, in the tabs on top mode, the background color of the nav-bar would be incorrect (see http://grab.by/iSNm for a screenshot.)  I think the reason that the :-moz-lwtheme code can get away with this is the background image set behind all chrome elements.

So I think my original patch is what we need to take here.
Attached image screenshot2
More info:
* Error console which is opened from private window.
* The popup window.
Attachment #700755 - Attachment is patch: false
Attachment #700755 - Attachment mime type: text/plain → image/png
(In reply to Tetsuharu OHZEKI [:saneyuki_s] from comment #11)
> Created attachment 700755 [details]
> screenshot2
> 
> More info:
> * Error console which is opened from private window.
> * The popup window.

This seems to be a separate issue.  Can you lease file another bug?  Thanks!
Dao: ping?
Blocks: 829870
(In reply to :Ehsan Akhgari from comment #12)
> This seems to be a separate issue.  Can you lease file another bug?  Thanks!

OK. I filed Bug 829870.
I seem this might be caused by the same root of this bug.
No longer blocks: 829870
Comment on attachment 699841 [details] [diff] [review]
Patch (v1)

>+[privatebrowsingmode=temporary] #nav-bar[tabsontop=false] {

#main-window[privatebrowsingmode=temporary]
Attachment #699841 - Flags: review?(dao) → review+
Comment on attachment 699841 [details] [diff] [review]
Patch (v1)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 749394
User impact if declined: See comment 0
Testing completed (on m-c, etc.): Locally
Risk to taking this patch (and alternatives if risky): not risky at all
String or UUID changes made by this patch: none
Attachment #699841 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/838a17192070
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 21
Comment on attachment 699841 [details] [diff] [review]
Patch (v1)

low risk regression fix, approving for aurora.
Attachment #699841 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:20.0) Gecko/20100101 Firefox/20.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:20.0) Gecko/20100101 Firefox/20.0

Verified as fixed in Firefox 20 beta 6 (buildID: 20130320062118).
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.