Notification Bar Changes #6939

jonathansampson · 2017-01-31T07:15:30Z

Submitted a ticket for my issue if one did not already exist.
Used Github auto-closing keywords in the commit message.
Added/updated tests for this change (for new code or code which already has tests).
Ran git rebase -i to squash commits (if needed).

Resolves #6935

luixxiul · 2017-01-31T11:54:43Z

less/notificationBar.less

@@ -7,7 +7,19 @@

 .notificationBar {
  display: inline-block;
+  position: relative;
+  z-index: 1000;


I'd replace this with @zindexNavigationBar according to

browser-laptop/less/variables.less

Line 129 in ce25254

@zindexNavigationBar: 2000;

@luixxiul That appears to cause the type of problems we faced in the design discussion last night. Namely, interacting with some of the document elements will cause the shadow to vanish; often only regions of it will vanish.

How about changing @zindexNavigationBar on variables.less into 1000 then??

bsclifton · 2017-01-31T20:52:42Z

@jonathansampson is this one ready for review? If not, can we move it to 0.13.3? thanks 😄

jonathansampson · 2017-01-31T20:54:47Z

@bsclifton I'm not positive; @bradleyrichter, thoughts?

bradleyrichter · 2017-01-31T23:12:54Z

I thought we were stuck at the lame-triangle problem? Did you find a solution @jonathansampson ?

jonathansampson · 2017-01-31T23:25:21Z

@bradleyrichter I'm looking into some of the tab-change, window-resize events now to see if we can just subscribe to those and pick up the needed information to position the triangle. Positioning it will require us to use a true element, rather than a pseudo element.

cezaraugusto · 2017-02-23T18:31:31Z

I have a fix for triangle issue and going to self-assign this and make a follow-up of @jonathansampson changes.

cezaraugusto · 2017-03-14T20:01:41Z

@jonathansampson I edited PR with caret feel free to edit anything. Didn't test all use cases for app-wise notifications, tried only fullscreen

jonathansampson · 2017-03-15T18:09:28Z

@cezaraugusto Wonder if we should be concerned with the following scenario: A user has multiple tabs opened, and may confuse the notification on one for being a notification on another. Note below how the notification bars for github.io appear to be coming from google.com.

Another question is can a user change the favicon to appear as though it has a triangle in it, thus making the spoofing more persuasive to many users. We may want to get @diracdeltas' input here as well.

diracdeltas · 2017-03-15T18:41:20Z

@jonathansampson the problem with having the notification bars fully in the webview area is that a page can spoof them; this is why initially i implemented them to be above the tab bar. this is solved by having the triangle poke into the "trusted UI" area, but yeah i agree it's confusing to have it point to a different tab. IMO it should always point to either the URL bar or the tab where the notification is coming from.

bradleyrichter · 2017-03-15T18:54:13Z

This should never happen anyway because we have switched the alerts to be tab specific.

…

On Mar 15, 2017, at 11:41 AM, yan ***@***.***> wrote: @jonathansampson <https://github.com/jonathansampson> the problem with having the notification bars fully in the webview area is that a page can spoof them; this is why initially i implemented them to be above the tab bar. this is solved by having the triangle poke into the "trusted UI" area, but yeah i agree it's confusing to have it point to a different tab. IMO it should always point to either the URL bar or the tab where the notification is coming from. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#6939 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AM4jqlU1PqrjUfEqyCZK3_qZt5UvEWLKks5rmDDUgaJpZM4LyTqb>.

cezaraugusto · 2017-03-15T20:38:14Z

yes that's my fault, didn't test for more than one alert. I'll do an update.

To avoid spoofing we can move the caret to the close button instead of favicon, so we can back to safe UI area. When tab is small enough to remove close icon we could also remove the caret.

/cc @jonathansampson @bradleyrichter @diracdeltas

diracdeltas · 2017-03-15T20:48:50Z

@bradleyrichter i don't know what you mean. the notifications right now appear in the ("global") trusted UI area, but they only appear when the tab they are for is active.

@cezaraugusto as long as the caret isn't going to an unrelated tab, sgtm

bsclifton · 2017-03-15T20:51:46Z

@cezaraugusto if the notifications are per-tab, you'll also be able to close this issue out 😄 #7737

jonathansampson · 2017-03-15T21:19:48Z

@bradleyrichter @diracdeltas In the above screenshot, Google.com has a notification bar. I left it untouched, and moved over to webrtc.github.io, where two additional notification bars were opened. Each bar points only to its related tab. But triangles are visible even when associated notification bars are not.

@cezaraugusto I would suggest placing the triangle in the center of the tab rather than on the left or right. Should we also hide the triangle if the tab isn't focused? Or, perhaps mark tabs in such a manner so as to show that they have a pending notification? Something other than the triangle for blurred tabs?

diracdeltas · 2017-03-15T21:47:53Z

@jonathansampson i see, i didn't realize there was a notification for the goog tab. in that case seems like the triangle for google should just disappear (or turn grey)

cezaraugusto · 2017-03-15T22:15:58Z

edited, now caret only appears if tab is active and has a notificationBar. Also moved caret to the center.

jonathansampson · 2017-03-15T22:56:59Z

js/components/main.js

          hasTabInFullScreen={
            sortedFrames
              .map((frame) => frame.get('isFullScreen'))
              .some(fullScreenMode => fullScreenMode === true)
          }
        />
+        {
+          this.props.appState.get('notifications') && this.props.appState.get('notifications').size && activeFrame


We call out to this.props.appState.get('notifications') 3-4 times. Can we do this once, and then store the result?

you mean adding to a variable? If so, we can:

const notifications = this.props.appState.get('notifications') const hasNotifications = notifications && notifications.size ... hasNotifications && activeFrame ? <NotificationBar notifications={notifications} activeFrame={activeFrame} /> : null

@cezaraugusto Yes. This way we only have to go searching for the object once.

bradleyrichter · 2017-03-16T06:22:57Z

And I confused a pair of tab alerts for separate tab alerts. Sorry! So, I confirmed we are good with tab specific alerts. Next we need to stack them in z order and not y order. And hide the non-active nub as Yan mentioned.

…

On Mar 15, 2017, at 2:47 PM, yan ***@***.***> wrote: @jonathansampson i see, i didn't realize there was a notification for the goog tab. in that case seems like the triangle for google should just disappear (or turn grey) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

luixxiul · 2017-03-23T14:18:57Z

I'm just wondering, if we really need the caret over the tab title. I can easily imagine a bunch of users would say, "please add an option to hide it or just remove it because I want to read the full title".

For me the aim of the caret is unclear, because, if I select a tab and there appears a notification bar under the tab, it is obvious that the notification bar appears there exactly for the tab which I selected just now.

Also, in this case, if the tab preview is enabled, the notification bar should be hidden if another tab is previewed which do not have notification.

diracdeltas · 2017-03-23T15:21:16Z

The caret is needed because otherwise a notification bar can be spoofed by a malicious page.

diracdeltas · 2017-03-23T15:22:12Z

The way chrome/ff handle this is by having a notification box pop up from the URL bar when the tab it is for is in focus. That would be better.

cezaraugusto · 2017-03-23T15:53:38Z

I'm ok either way change it to be nested to urlbar should be minimal

cezaraugusto · 2017-04-05T17:37:35Z

@bradleyrichter ok to use it the same way as Chrome/FF?

bradleyrichter · 2017-04-05T17:40:22Z

I am not sure this would work because it would cover all of the other tabs, forcing the user to interact with the message before using other tabs?

bradleyrichter · 2017-04-05T17:41:22Z

Let's try adding a border and making the nub more defined and obvious.

bsclifton · 2017-04-19T05:40:01Z

How's this one looking? @jonathansampson, @cezaraugusto, @luixxiul

jonathansampson · 2017-04-19T06:38:23Z

@bsclifton I'm assuming it's ready to go since @cezaraugusto added PR/ready-for-review a day ago. @bradleyrichter, how do you feel about the present presentation?

NejcZdovc · 2017-04-19T09:48:03Z

js/components/notificationBar.js

@@ -115,6 +116,14 @@ class NotificationBar extends ImmutableComponent {
  }
 }

+class NotificationBarCaret extends ImmutableComponent {


Why do we need this component? I don't see real benefit of having it.

@NejcZdovc What do you suggest as an alternative?

I am not sure if we need component for two divs. What I would suggest is to create divs where we now use this component and move styles into notificationStyle file and use it from there.
cc @cezaraugusto

I prefer this way because then I can leave styles on the component itself. Otherwise, I'll have to create a new style for that and put it on the tab itself or inside a third file, and if I do it then I'm not taking advantage of Aphrodite.

Style and component should live together so I don't have to run several files to find a simple thing. Things we do such as commonStyles.js is just a hack we found to keep refactoring important stuff while common components aren't ready yet/too nested. Having both together makes regressions less likely.

It also simplifies the code, so if for some reason Brad or other less code-experienced member/collaborator wants to change something for a given component, they'll need to go to into just one file.

bradleyrichter · 2017-04-19T21:25:20Z

I'm happy now. Let's merge and refine the styles separately as @NejcZdovc suggested.

cezaraugusto · 2017-04-20T04:05:54Z

Based on latest Brad's spec:


        Make notificationBar tab-specific

@bradleyrichter

Auditors: @bradleyrichter, @jonathansampson Close #6935

jonathansampson · 2017-04-20T16:37:45Z

@cezaraugusto Holy smokes, that's gorgeous! :)

srirambv · 2017-05-02T15:28:56Z

Nub not shown for pinned tabs

Also notification for pinned tabs is retained accross normal tabs as well. This is on packaged 0.15.2 packaged preview 1 build

bradleyrichter · 2017-05-02T21:31:01Z

it should also work the same for pinned tabs.

jonathansampson added the design label Jan 31, 2017

jonathansampson added this to the 0.13.2 milestone Jan 31, 2017

luixxiul reviewed Jan 31, 2017

View changes

bbondy modified the milestones: 0.13.3, 0.13.2 Feb 1, 2017

bbondy removed this from the 0.13.5 milestone Feb 16, 2017

cezaraugusto self-assigned this Feb 23, 2017

luixxiul added the PR/needs-rebase ۞ label Feb 23, 2017

cezaraugusto force-pushed the notification-bar branch from 4def2bd to 75152fb Mar 14, 2017

cezaraugusto removed the PR/needs-rebase ۞ label Mar 14, 2017

cezaraugusto added the PR/work-in-progress ⚒ label Mar 14, 2017

cezaraugusto force-pushed the notification-bar branch from 75152fb to 64d6c15 Mar 15, 2017

jonathansampson reviewed Mar 15, 2017

View changes

cezaraugusto force-pushed the notification-bar branch from 64d6c15 to 017429f Mar 17, 2017

cezaraugusto force-pushed the notification-bar branch 2 times, most recently from c2a5005 to ae2805e Apr 17, 2017

cezaraugusto added PR/needs-QA-attention ☕ and removed PR/work-in-progress ⚒ labels Apr 18, 2017

NejcZdovc reviewed Apr 19, 2017

View changes

cezaraugusto force-pushed the notification-bar branch 3 times, most recently from fe72678 to e5ecc26 Apr 20, 2017

Make notificationBar tab-specific

Verified

This commit was signed with a verified signature.

cezaraugusto Cezar Augusto

GPG key ID: ECC5FF1C7EA1D39D Learn about signing commits

Loading status checks…

ffc2d9b

Auditors: @bradleyrichter, @jonathansampson Close #6935

cezaraugusto force-pushed the notification-bar branch from e5ecc26 to ffc2d9b Apr 20, 2017

cezaraugusto merged commit 1d8588d into master Apr 20, 2017
0 of 2 checks passed

0 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

luixxiul removed the PR/needs-QA-attention ☕ label Apr 20, 2017

cezaraugusto deleted the notification-bar branch Apr 20, 2017

NejcZdovc added this to the 0.15.1 milestone Apr 20, 2017

srirambv mentioned this pull request May 2, 2017

Pinned tab notification doesn't have the caret and shown across all tabs #8626

Closed

brave / browser-laptop Archived

Join GitHub today

Notification Bar Changes #6939

Notification Bar Changes #6939

Conversation

jonathansampson commented Jan 31, 2017

This comment has been minimized.

luixxiul Jan 31, 2017 • edited

This comment has been minimized.

This comment has been minimized.

bsclifton commented Jan 31, 2017

jonathansampson commented Jan 31, 2017

bradleyrichter commented Jan 31, 2017

jonathansampson commented Jan 31, 2017

cezaraugusto commented Feb 23, 2017

cezaraugusto commented Mar 14, 2017

jonathansampson commented Mar 15, 2017 • edited

diracdeltas commented Mar 15, 2017

bradleyrichter commented Mar 15, 2017

cezaraugusto commented Mar 15, 2017

diracdeltas commented Mar 15, 2017

bsclifton commented Mar 15, 2017

jonathansampson commented Mar 15, 2017 • edited

diracdeltas commented Mar 15, 2017

cezaraugusto commented Mar 15, 2017

This comment has been minimized.

This comment has been minimized.

cezaraugusto Mar 16, 2017 • edited

This comment has been minimized.

bradleyrichter commented Mar 16, 2017

luixxiul commented Mar 23, 2017

diracdeltas commented Mar 23, 2017

diracdeltas commented Mar 23, 2017

cezaraugusto commented Mar 23, 2017

cezaraugusto commented Apr 5, 2017

bradleyrichter commented Apr 5, 2017

bradleyrichter commented Apr 5, 2017

bsclifton commented Apr 19, 2017

jonathansampson commented Apr 19, 2017 • edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

bradleyrichter commented Apr 19, 2017

cezaraugusto commented Apr 20, 2017

jonathansampson commented Apr 20, 2017

srirambv commented May 2, 2017

bradleyrichter commented May 2, 2017

luixxiul Jan 31, 2017 •

edited

jonathansampson commented Mar 15, 2017 •

edited

jonathansampson commented Mar 15, 2017 •

edited

cezaraugusto Mar 16, 2017 •

edited

jonathansampson commented Apr 19, 2017 •

edited