[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.

tracking captive portal related issue for 52

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.

(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?

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.

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.

(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.

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

(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?

(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.

(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).

(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.

(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".

(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.

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.

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

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

Will you request uplift for this patch?

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]:

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+

Verified fixed on Ubuntu 14.04 x64 using Firefox 52 Beta 8 (buildID: 20170220070057) and latest Aurora 53.0a2 (buildID: 20170222084056).