598331 - Stop using a single notificationbox for all browsers

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Mark Finkle (:mfinkle) (use needinfo?)]

Now that we are using real browsers, we should switch our XUL a bit and use 1 notificationbox per browser. Right now, all browsers share a single notification box and it causes problems trying to keep the actual notifications working for each browser.

Comment 1

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 477482 [details] [diff] [review]
Patch

Looks good, but I think we can remove more from notification.xml

The <constructor>, <method name="handleEvent"> and the | document.removeEventListener | in <method name="close"> can be removed.

r- for the notification.xml stuff

Comment 3

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to comment #2)
> Comment on attachment 477482 [details] [diff] [review]
> Patch
> 
> Looks good, but I think we can remove more from notification.xml
> 
> The <constructor>, <method name="handleEvent"> and the |
> document.removeEventListener | in <method name="close"> can be removed.

Is it not what I've done?
/me has a doubt

By the way, sorry Wes for posting this patch, I've not seen your name in the assignee field, I need to change my glasses!

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 477482 [details] [diff] [review]
Patch

/me failed to scroll down far enough :(

Comment 5

[Wesley Johnston (:wesj)]

This will cause problems with inputhandler-overlay and tap-highlights. Vivien was nice enough to let me try and fix it.

There isn't really any nice way that I can think of move input handler (or the tap-highlight overlay) inside the notificationbox without making them per-browser. So I'm trying to catch events and fix 'em. I'm not missing something, am I?

Comment 6

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to comment #5)
> This will cause problems with inputhandler-overlay and tap-highlights. Vivien
> was nice enough to let me try and fix it.
> 
> There isn't really any nice way that I can think of move input handler (or the
> tap-highlight overlay) inside the notificationbox without making them
> per-browser. So I'm trying to catch events and fix 'em. I'm not missing
> something, am I?

I think you should make them handle events as if they were regular chrome UI (and this is what these notifications are) because for now the path for dispatching event to content is probably activated.
Basicaly in the old InputHandler.js there was 2 paths, one for the Chrome UI, the other for Content, and these notifications should take the first one (not sure there is still 2 paths with the change from tonight though)

Comment 7

[Wesley Johnston (:wesj)]

Attachment: Unbitrot

Just a quick unbitrot

Comment 8

[Wesley Johnston (:wesj)]

Attachment: Fix up input stuff

This is the input stuff. Not sure if this is what we want or not though, so I'll ask for feedback.

Basically this moves the input handler and tap-highlight inside the notificationbox binding. targetIsContent should then check to see if the event was on the browser area... but... none of the event targets seems to tell me that. So I settled for just checking if it was on the notification box for now. I also moved targetIsContent out of the click helper, which isn't really necessary, but I like having it out of there anyway.

We also have to redirect the ContentCustomKeySender, customDragger, and ScrollwheelModule events, so I just attached them to the deck containing the browsers (customDragger required some screwy hacky stuff to get that to work). customDragger is what I kinda wanted feedback on. Pinch zoom is still broken with this. Looking into that.

I also moved the tap highlighter overlay into the binding, and fiddled with that code a bit. I understand it was the way it was for some performance reason. Performance is actually fine on my Droid with it this way, but that change shouldn't be necessary. Mostly me just playing around.

Comment 9

[Wesley Johnston (:wesj)]

Attachment: Input stuff - unbitrotted

Comment 10

[Wesley Johnston (:wesj)]

Attachment: Input stuff un-bitrot... agains?

Comment 11

[Benjamin Stover (:stechz)]

Comment on attachment 478434 [details] [diff] [review]
Input stuff un-bitrot... agains?

>   show: function show(aRects) {
>     let browser = getBrowser();
>     let scroll = browser.getPosition();
> 
>-    let canvasArea = aRects.reduce(function(a, b) {
>-      return a.expandToContain(b);
>-    }, new Rect(0, 0, 0, 0)).map(function(val) val * browser.scale)
>-                            .translate(-scroll.x, -scroll.y);
>-
>+    let canvasArea = browser.getBoundingClientRect();
>     let overlay = this._overlay;
>     overlay.width = canvasArea.width;
>     overlay.style.width = canvasArea.width + "px";
>     overlay.height = canvasArea.height;
>     overlay.style.height = canvasArea.height + "px";
> 
>     let ctx = overlay.getContext("2d");
>     ctx.save();
>-    ctx.translate(-canvasArea.left, -canvasArea.top);
>+    ctx.translate(-scroll.x, -scroll.y);
>     ctx.scale(browser.scale, browser.scale);
> 
>-    overlay.style.left = canvasArea.left + "px";
>-    overlay.style.top = canvasArea.top + "px";
>     ctx.fillStyle = "rgba(0, 145, 255, .5)";
>     for (let i = aRects.length - 1; i >= 0; i--) {
>       let rect = aRects[i];
>-      ctx.fillRect(rect.left - scroll.x / browser.scale, rect.top - scroll.y / browser.scale, rect.width, rect.height);
>+      ctx.fillRect(rect.left, rect.top, rect.width, rect.height);
>     }
>     ctx.restore();
>     overlay.style.display = "block";
> 
>     addEventListener("MozBeforePaint", this, false);
>     mozRequestAnimationFrame();
>   },

I'm concerned about perf here, since we now have a fullscreen translucent canvas. I haven't tried this on device yet, but I'd especially want to make sure this doesn't regress N900. Also, when notification boxes are visible we don't get any link highlighting anymore.

>     let container = document.getElementById("browsers");
>     // XXX change
> 
>     /* handles dispatching clicks on browser into clicks in content or zooms */
>-    let inputHandlerOverlay = document.getElementById("inputhandler-overlay");
>-    inputHandlerOverlay.customDragger = new Browser.MainDragger();
>-
>-    let keySender = new ContentCustomKeySender(inputHandlerOverlay);
>+    let contentBox = document.getElementById("browsers");

container and contentBox are the same thing here.

>+  },
>+
>+  /**
>+   * Check if the event concern the browser content
>+   */
>+  targetIsContent: function _targetIsContent(aEvent) {
>+    let target = aEvent.originalTarget;
>+    dump(target.nodeName + "\n");
>+    return target && target.nodeName == "notificationbox";
>+  },
> };

Hmm, it looks like we no longer need to move this to Browser? Only CustomTouchHandler uses it.

>+  get inputHandler() {
>+    return this._notification.inputHandler;
>+  },
>+  
>+  get overlay() {
>+    return this._notification.overlay;    
>+  },

I'm not great at names, but I am questioning "inputHandler" name. Maybe you or Mark have an idea here? It's a nice opportunity to change it.

BTW, are you using good hgrc settings? See https://developer.mozilla.org/en/Installing_Mercurial (particularly, git=1 showfunc=1 unified=8).

>-              <stack id="tile-stack" class="window-width" flex="1">
>-              <!-- Content viewport -->
>-                <html:div>
>-                  <html:div id="browsers"/>
>-                  <html:canvas id="content-overlay" style="display: none; position: absolute; z-index: 1000; left: 0; top: 0;"/>
>-                </html:div>
>-                <html:div id="inputhandler-overlay" style="z-index: 1001" tabindex="-1"/>
>-              </stack>
>+              <deck id="browsers"/>

Very nice :)

>         if (elem.scrollBoxObject) {
>           scrollbox = elem;
>           qinterface = elem.scrollBoxObject;
>           break;
>         } else if (elem.boxObject) {
>+          if(elem.customDragger) {
>+            scrollbox = elem;
>+            break;
>+          }

Just move the else if (elem.customDragger) check below to the top, so no matter what customDragger wins.

One other thing I noticed: when notification is shown in one tab, it changes the height of other tabs. Not sure how decks are supposed to work so I don't know why this happens.

This patch is shaping up really well! Let's make sure we haven't regressed on tap highlight performance and fix these bugs.

Comment 12

[Wesley Johnston (:wesj)]

Attachment: Move to one Notification 1/2

Part 1 - Vivien's patch unrotted

Comment 13

[Wesley Johnston (:wesj)]

Attachment: Input fixes (2/2)

This should work. I put off the highlight changes, although I had to make a few tweaks to get things to work here. The other changes can be a different bug if we ever want them. I have a feeling we don't though. I still hate the nodeName = "notificationbox" stuff. If there's a better way (checking for a class?), I would like to hear it.

I'm also curious about moving the Tab object into the notificationbox binding, and using the deck to keep track of selectedBrowser/selectedTab. Not sure what that gets us except that I feel like its cleaner.

Comment 14

[Benjamin Stover (:stechz)]

> Also, when notification boxes are visible we
> don't get any link highlighting anymore.

Did this problem get fixed?

I still want to see this on N900 before we take it. I can get a build running on the N900 and see how this does.

Comment 15

[Benjamin Stover (:stechz)]

> This should work. I put off the highlight changes, although I had to make a few
> tweaks to get things to work here. The other changes can be a different bug if
> we ever want them. I have a feeling we don't though. I still hate the nodeName
> = "notificationbox" stuff. If there's a better way (checking for a class?), I
> would like to hear it.

Sure, you could just check for class. className == "forward-content" perhaps?

> I'm also curious about moving the Tab object into the notificationbox binding,
> and using the deck to keep track of selectedBrowser/selectedTab. Not sure what
> that gets us except that I feel like its cleaner.

We might as well use <tabbrowser> element then? This is worth a try IMO, but I we'll have to watch for regressions.

Comment 16

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #15)
> > This should work. I put off the highlight changes, although I had to make a few
> > tweaks to get things to work here. The other changes can be a different bug if
> > we ever want them. I have a feeling we don't though. I still hate the nodeName
> > = "notificationbox" stuff. If there's a better way (checking for a class?), I
> > would like to hear it.
> 
> Sure, you could just check for class. className == "forward-content" perhaps?

Use the new classList API in case we use several classes

> > I'm also curious about moving the Tab object into the notificationbox binding,
> > and using the deck to keep track of selectedBrowser/selectedTab. Not sure what
> > that gets us except that I feel like its cleaner.
> 
> We might as well use <tabbrowser> element then? This is worth a try IMO, but I
> we'll have to watch for regressions.

I hope you're joking about <tabbrowser>, it is far to desktop Firefox centric now. In fact, we'd need to copy it into Fennec to even work.

If we move anything into a binding, we would use the tabs.xml bindings for the <tablist> and <tab>

Comment 17

[Benjamin Stover (:stechz)]

First patch is no longer applying for me. Can I get another unbitrotted version? Then I can verify if this is performing alright on the N900.

Comment 18

[Wesley Johnston (:wesj)]

Attachment: Remove notifications (1/2)

Comment 19

[Wesley Johnston (:wesj)]

Attachment: Input fixes (2/2)

Unbitrotted

Comment 20

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 481511 [details] [diff] [review]
Input fixes (2/2)

I'm not a fan of "inputHandler" the property and the classname. The <div> doesn't handle any input. It is an "input-overlay" more than anything. I guess "inputOverlay" is an ok property name. "content-overlay" is really the "highlight-overlay", right?

Comment 21

[Mark Finkle (:mfinkle) (use needinfo?)]

Is this patch affected by bug 602708 as well?

Comment 22

[Wesley Johnston (:wesj)]

No problem changing the name. Yes, this is also affected by bug 602708. The patch there will likely need to be modified, as it sets:

deck.selectedPanel = selectedTab.browser

and here we need

deck.selectedPanel = selectedTab.notification

Comment 23

[Wesley Johnston (:wesj)]

Attachment: UnBitrot - Remove notifications (1/2)

Comment 24

[Wesley Johnston (:wesj)]

Attachment: UnBitrot - Input fixes (2/2)

Cleaned up one more time.

Comment 25

[Benjamin Stover (:stechz)]

Comment on attachment 486748 [details] [diff] [review]
UnBitrot - Input fixes (2/2)

>-    if (!success || (aEvent.target instanceof XULElement) ||
>-        !Browser.selectedTab.allowZoom)
>+    if (!success || (aEvent.target instanceof XULElement) || !Browser.selectedTab.allowZoom)
>       return;

More readable for me on two lines.

Tap delay seems fine on N900. r+

Comment 26

[Mark Finkle (:mfinkle) (use needinfo?)]

* Renamed Tab.updateBrowser method -> Tab.active property
* Fixed some errors by doing some null checks in Tab properties
* Fixed browser-chrome tests, changing Browser.selectedTab.browser -> Browser.selectedTab.notification in some places.

pushed:
http://hg.mozilla.org/mobile-browser/rev/f83003b34fbe
http://hg.mozilla.org/mobile-browser/rev/af72df54ccf1

Comment 27

[Matt Brubeck (:mbrubeck)]

This seems to have broken pinch zoom: bug 611220

Comment 28

[Matt Brubeck (:mbrubeck)]

Backed out until the regression is fixed:
http://hg.mozilla.org/mobile-browser/rev/3b0a25f0aab2

Comment 29

[Matt Brubeck (:mbrubeck)]

Attachment: fix pinch zoom regression

I think this is the right fix for the regression.

Comment 30

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 489735 [details] [diff] [review]
fix pinch zoom regression

Talked on IRC. The changes make sense.

Comment 31

[Mark Finkle (:mfinkle) (use needinfo?)]

Notes from Matt:
* We used to check (aEvent.target instanceof XULElement) to exclude any XUL elements, but now the target is an <xul:deck> [This is the main bustage]
* We don't want to zoom the browser if you pinch on the urlbar, or the sidebar, or the tool panel, so we add event listeners to the browsers, not the window

Comment 32

[Matt Brubeck (:mbrubeck)]

Re-landed with the zoom fix:
http://hg.mozilla.org/mobile-browser/rev/ae80e7d65959
http://hg.mozilla.org/mobile-browser/rev/a995c587298c
http://hg.mozilla.org/mobile-browser/rev/c996bd884dee

Comment 33

[Anna (Waverley)]

VERIFIED FIXED on:

Mozilla /5.0 (Android;Linux armv7l;rv:7.0a1) Gecko/20110602 Firefox/7.0a1 Fennec/7.0a1 

Mozilla /5.0 (Android;Linux armv7l;rv:6.0a2) Gecko/20110601 Firefox/6.0a2 Fennec/6.0a2 

Device: HTC Desire Z (Android)