[Captive Portal] Notification bar misses border in Ubuntu

VERIFIED FIXED in Firefox 52

Status

()

Firefox
General
--
minor
VERIFIED FIXED
6 months ago
3 months ago

People

(Reporter: AdrianSV, Assigned: nhnt11)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 53
All
Linux
Points:
---

Firefox Tracking Flags

(firefox50 unaffected, firefox51 unaffected, firefox52+ verified, firefox53 verified)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Reporter)

Description

6 months ago
Created attachment 8814159 [details]
Notification bar border.png

[Enviroment:]
OSes:
-Ubuntu 16.04 

53.0a1 Build ID  20161121074938   

[Description]: The Notification bar looks unpolished in Ubuntu. 


[Steps]:
    1. Disconnect all network connections.
    2. Enable wifi and attempt connecting to a Captive portal SSID. (do not finalize the connection by actually logging in)
    3. Start FF. 
    4. When Captive Portal Notification  bar is displayed, notice the bar border missing.
    
    
[Actual Result]:
     See the attached screenshot with how the Notification bar looks on Ubuntu(without border) vs Win10 and Mac OS X.

[Expected Result]:
     The border should be present for the Notification bar in Ubuntu as well.


[Note:]
Additionally, the Show login button in Windows looks unpolished.
[Tracking Requested - why for this release]: Visual regression exposed by captive portal.
status-firefox50: --- → unaffected
status-firefox51: --- → unaffected
status-firefox52: --- → affected
status-firefox53: --- → affected
tracking-firefox52: --- → ?
tracking captive portal related issue for 52
tracking-firefox52: ? → +
Comment hidden (mozreview-request)
(Assignee)

Comment 4

6 months ago
mozreview-review
Comment on attachment 8816330 [details]
Bug 1320128 - [Captive Portal] Add bottom/top borders to info notifications on Linux.

https://reviewboard.mozilla.org/r/97094/#review97316

::: toolkit/themes/linux/global/notification.css:20
(Diff revision 1)
>  
>  notification[type="info"] {
>    color: -moz-DialogText;
>    background-color: -moz-Dialog;
>    -moz-appearance: none;
> +  border-bottom: 1px solid ThreeDShadow;

This is identical to the bottom border of the toolbar.
(Assignee)

Updated

6 months ago
Assignee: nobody → nhnt11

Comment 5

6 months ago
(In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from comment #1)
> [Tracking Requested - why for this release]: Visual regression exposed by
> captive portal.

What regressed this?
Flags: needinfo?(MattN+bmo)

Comment 6

6 months ago
mozreview-review
Comment on attachment 8816330 [details]
Bug 1320128 - [Captive Portal] Add bottom/top borders to info notifications on Linux.

https://reviewboard.mozilla.org/r/97094/#review97460

r- without further details, because when we went over this in bug 1162635, Dão was sure that the native linux styling should be taking care of the borders. See https://bugzilla.mozilla.org/show_bug.cgi?id=1162635#c21 . Please explain why we suddenly need this / how this regressed / what changed (did our gtk implementation change?) and request the next review from Dão.
Attachment #8816330 - Flags: review?(gijskruitbosch+bugs) → review-

Comment 7

6 months ago
mozreview-review-reply
Comment on attachment 8816330 [details]
Bug 1320128 - [Captive Portal] Add bottom/top borders to info notifications on Linux.

https://reviewboard.mozilla.org/r/97094/#review97460

Also, just always adding a bottom border would be wrong for notifications that show up at the bottom, giving them a double border, so even if borders are missing a slightly more elaborate patch would be necessary to fix this correctly.
(Assignee)

Comment 8

6 months ago
(In reply to :Gijs Kruitbosch from comment #6)
> Comment on attachment 8816330 [details]
> Bug 1320128 - [Captive Portal] Add bottom border to info notifications on
> Linux.
> 
> https://reviewboard.mozilla.org/r/97094/#review97460
> 
> r- without further details, because when we went over this in bug 1162635,
> Dão was sure that the native linux styling should be taking care of the
> borders. See https://bugzilla.mozilla.org/show_bug.cgi?id=1162635#c21 .
> Please explain why we suddenly need this / how this regressed / what changed
> (did our gtk implementation change?) and request the next review from Dão.

Dão says that because -moz-appearance is set to -moz-gtk-info-bar for the notification class, but it's set to none for notification[type="info"]. I'll investigate whether we want to change -moz-appearance for notification[type="info"] or just add the border like I've already done.
(Assignee)

Comment 9

5 months ago
Hmm, seems like on both macOS and Windows, all notifications have a 1px border-{bottom, top} [1, 2]. This is missing on Linux; however, there is already a border above the notification on Linux (from [3]) so we definitely don't want to add another border-top. I propose we add a border-bottom to the notificationbox itself, rather than individual notifications. I'll attach a patch to this effect.

[1] https://dxr.mozilla.org/mozilla-central/rev/77eebb6c80cd5249b9a43bbcdefdaaa9d9845d1e/toolkit/themes/osx/global/notification.css#16 (also #23 and #30)
[2] https://dxr.mozilla.org/mozilla-central/rev/77eebb6c80cd5249b9a43bbcdefdaaa9d9845d1e/toolkit/themes/windows/global/notification.css#11
[3] https://dxr.mozilla.org/mozilla-central/rev/77eebb6c80cd5249b9a43bbcdefdaaa9d9845d1e/browser/themes/linux/browser.css#61
Comment hidden (mozreview-request)
(Assignee)

Updated

5 months ago
Status: NEW → ASSIGNED

Comment 11

5 months ago
(In reply to Nihanth Subramanya [:nhnt11] from comment #9)
> Hmm, seems like on both macOS and Windows, all notifications have a 1px
> border-{bottom, top} [1, 2]. This is missing on Linux; however, there is
> already a border above the notification on Linux (from [3]) so we definitely
> don't want to add another border-top. I propose we add a border-bottom to
> the notificationbox itself, rather than individual notifications. I'll
> attach a patch to this effect.
> 
> [1]
> https://dxr.mozilla.org/mozilla-central/rev/
> 77eebb6c80cd5249b9a43bbcdefdaaa9d9845d1e/toolkit/themes/osx/global/
> notification.css#16 (also #23 and #30)
> [2]
> https://dxr.mozilla.org/mozilla-central/rev/
> 77eebb6c80cd5249b9a43bbcdefdaaa9d9845d1e/toolkit/themes/windows/global/
> notification.css#11
> [3]
> https://dxr.mozilla.org/mozilla-central/rev/
> 77eebb6c80cd5249b9a43bbcdefdaaa9d9845d1e/browser/themes/linux/browser.css#61

Doesn't this mean that in the notificationbox at the bottom, if you were to use a type=info notification, it would have no top border? In other words, based on comment #8, shouldn't we add a border-top and bottom and hide the one that doesn't apply for notifications at the top/bottom, like we do on OS X?
Flags: needinfo?(MattN+bmo) → needinfo?(nhnt11)
Comment hidden (mozreview-request)
(Assignee)

Comment 13

5 months ago
(In reply to :Gijs from comment #11)
> Doesn't this mean that in the notificationbox at the bottom, if you were to
> use a type=info notification, it would have no top border? In other words,
> based on comment #8, shouldn't we add a border-top and bottom and hide the
> one that doesn't apply for notifications at the top/bottom, like we do on OS
> X?

You're right, there's no top border currently for info notifications in the box at the bottom. Latest patch copies what macOS and Windows are doing. Seems like we're setting the border directly on individual notifications instead of the container because the border colour is different for different notification types on macOS.
Flags: needinfo?(nhnt11)

Comment 14

5 months ago
(In reply to Nihanth Subramanya [:nhnt11] from comment #13)
> (In reply to :Gijs from comment #11)
> > Doesn't this mean that in the notificationbox at the bottom, if you were to
> > use a type=info notification, it would have no top border? In other words,
> > based on comment #8, shouldn't we add a border-top and bottom and hide the
> > one that doesn't apply for notifications at the top/bottom, like we do on OS
> > X?
> 
> You're right, there's no top border currently for info notifications in the
> box at the bottom. Latest patch copies what macOS and Windows are doing.
> Seems like we're setting the border directly on individual notifications
> instead of the container because the border colour is different for
> different notification types on macOS.

This sets the border everywhere instead of just for type=info... That seems likely to either be useless or to override OS styling (at least in the gtk3 case), per earlier comments on this bug, for non-type=info notifications.

Feels like we should set the border for type=info and set border-top/bottom-style: none for all of the notifications (according to on which side they appear).
Flags: needinfo?(nhnt11)

Comment 15

5 months ago
(In reply to :Gijs from comment #14)
> Feels like we should set the border for type=info and set
> border-top/bottom-style: none for all of the notifications (according to on
> which side they appear).

Actually, maybe even removing the borders should be specific to type=info notifications in order to avoid destroying e.g. fake 3d styling with ThreeDShadow-style "3d" borders.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 18

5 months ago
(In reply to :Gijs from comment #14)
> This sets the border everywhere instead of just for type=info... That seems
> likely to either be useless or to override OS styling (at least in the gtk3
> case), per earlier comments on this bug, for non-type=info notifications.
> 
> Feels like we should set the border for type=info and set
> border-top/bottom-style: none for all of the notifications (according to on
> which side they appear).

Oh, duh. The whole point was that -moz-appearance takes care of the borders for warnings. New patch just adds a top border to notifications at the bottom and a bottom border to notifications at the top. Do you think we should add both borders by default and remove the unnecessary one in another rule? That would remove the requirement for the notification to be in a notificationbox with a notificationside attribute set to "top" or "bottom".

Comment 19

5 months ago
(In reply to Nihanth Subramanya [:nhnt11] from comment #18)
> (In reply to :Gijs from comment #14)
> > This sets the border everywhere instead of just for type=info... That seems
> > likely to either be useless or to override OS styling (at least in the gtk3
> > case), per earlier comments on this bug, for non-type=info notifications.
> > 
> > Feels like we should set the border for type=info and set
> > border-top/bottom-style: none for all of the notifications (according to on
> > which side they appear).
> 
> Oh, duh. The whole point was that -moz-appearance takes care of the borders
> for warnings. New patch just adds a top border to notifications at the
> bottom and a bottom border to notifications at the top. Do you think we
> should add both borders by default and remove the unnecessary one in another
> rule? That would remove the requirement for the notification to be in a
> notificationbox with a notificationside attribute set to "top" or "bottom".

Yeah, I think using the same logic as on other OSes would be sensible.
Flags: needinfo?(nhnt11)

Comment 20

5 months ago
mozreview-review
Comment on attachment 8816330 [details]
Bug 1320128 - [Captive Portal] Add bottom/top borders to info notifications on Linux.

https://reviewboard.mozilla.org/r/97094/#review103808

Clearing review for now given the comments on the bug.
Attachment #8816330 - Flags: review?(gijskruitbosch+bugs)
Comment hidden (mozreview-request)

Comment 22

5 months ago
mozreview-review
Comment on attachment 8816330 [details]
Bug 1320128 - [Captive Portal] Add bottom/top borders to info notifications on Linux.

https://reviewboard.mozilla.org/r/97094/#review104214
Attachment #8816330 - Flags: review?(gijskruitbosch+bugs) → review+
Comment hidden (mozreview-request)

Comment 24

4 months ago
Pushed by nhnt11@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/19328c5e2109
[Captive Portal] Add bottom/top borders to info notifications on Linux. r=Gijs

Comment 25

4 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/19328c5e2109
Status: ASSIGNED → RESOLVED
Last Resolved: 4 months ago
status-firefox53: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Will you request uplift for this patch?
Flags: needinfo?(nhnt11)
(Assignee)

Comment 27

4 months ago
Comment on attachment 8816330 [details]
Bug 1320128 - [Captive Portal] Add bottom/top borders to info notifications on Linux.

Approval Request Comment
[Feature/Bug causing the regression]: Captive Portal (not a regression though)
[User impact if declined]: Captive portal notification bar doesn't have a bottom border, looks ugly (more generally, this applies to all info notification bars)
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: just a CSS change to add a border
[String changes made/needed]:
Flags: needinfo?(nhnt11)
Attachment #8816330 - Flags: approval-mozilla-aurora?
Comment on attachment 8816330 [details]
Bug 1320128 - [Captive Portal] Add bottom/top borders to info notifications on Linux.

styling for captive portal notification on linux, aurora52+
Attachment #8816330 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Comment 29

4 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/b628307561a3
status-firefox52: affected → fixed
Verified fixed on Ubuntu 14.04 x64 using Firefox 52 Beta 8 (buildID: 20170220070057) and latest Aurora 53.0a2 (buildID: 20170222084056).
Status: RESOLVED → VERIFIED
status-firefox52: fixed → verified
status-firefox53: fixed → verified
You need to log in before you can comment on or make changes to this bug.