Stop using a single notificationbox for all browsers

VERIFIED FIXED

Status

Fennec Graveyard
General
VERIFIED FIXED
7 years ago
7 years ago

People

(Reporter: mfinkle, Assigned: wesj)

Tracking

Dependency tree / graph
Bug Flags:
in-testsuite ?

Details

Attachments

(3 attachments, 9 obsolete attachments)

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

Updated

7 years ago
tracking-fennec: --- → ?
(Reporter)

Updated

7 years ago
tracking-fennec: ? → 2.0+
(Assignee)

Updated

7 years ago
Assignee: nobody → wjohnston
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
Attachment #477482 - Flags: review?(mark.finkle) → review-
(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 on attachment 477482 [details] [diff] [review]
Patch

/me failed to scroll down far enough :(
Attachment #477482 - Flags: review- → review+
(Reporter)

Updated

7 years ago
Whiteboard: [fennec-checkin-postb1]
(Assignee)

Comment 5

7 years ago
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?
(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)
(Assignee)

Comment 7

7 years ago
Created attachment 478056 [details] [diff] [review]
Unbitrot

Just a quick unbitrot
Attachment #477482 - Attachment is obsolete: true
(Assignee)

Comment 8

7 years ago
Created attachment 478082 [details] [diff] [review]
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.
Attachment #478082 - Flags: feedback?(webapps)
(Assignee)

Comment 9

7 years ago
Created attachment 478314 [details] [diff] [review]
Input stuff - unbitrotted
Attachment #478082 - Attachment is obsolete: true
Attachment #478082 - Flags: feedback?(webapps)
(Assignee)

Comment 10

7 years ago
Created attachment 478434 [details] [diff] [review]
Input stuff un-bitrot... agains?
Attachment #478314 - Attachment is obsolete: true
Attachment #478434 - Flags: feedback?(webapps)
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.
Attachment #478434 - Flags: feedback?(webapps) → feedback+
(Assignee)

Comment 12

7 years ago
Created attachment 478622 [details] [diff] [review]
Move to one Notification 1/2

Part 1 - Vivien's patch unrotted
Attachment #478056 - Attachment is obsolete: true
Attachment #478622 - Flags: review+
(Assignee)

Comment 13

7 years ago
Created attachment 478624 [details] [diff] [review]
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.
Attachment #478434 - Attachment is obsolete: true
Attachment #478624 - Flags: review?(webapps)
(Reporter)

Updated

7 years ago
Whiteboard: [fennec-checkin-postb1]
> 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.
> 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.
(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>
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.
(Assignee)

Comment 18

7 years ago
Created attachment 481509 [details] [diff] [review]
Remove notifications (1/2)
Attachment #478622 - Attachment is obsolete: true
Attachment #481509 - Flags: review+
(Assignee)

Comment 19

7 years ago
Created attachment 481511 [details] [diff] [review]
Input fixes (2/2)

Unbitrotted
Attachment #478624 - Attachment is obsolete: true
Attachment #481511 - Flags: review?(webapps)
Attachment #478624 - Flags: review?(webapps)
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?
Is this patch affected by bug 602708 as well?
(Assignee)

Comment 22

7 years ago
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
(Assignee)

Comment 23

7 years ago
Created attachment 486747 [details] [diff] [review]
UnBitrot - Remove notifications (1/2)
Attachment #481509 - Attachment is obsolete: true
Attachment #486747 - Flags: review+
(Assignee)

Comment 24

7 years ago
Created attachment 486748 [details] [diff] [review]
UnBitrot - Input fixes (2/2)

Cleaned up one more time.
Attachment #481511 - Attachment is obsolete: true
Attachment #486748 - Flags: review?(ben)
Attachment #481511 - Flags: review?(ben)
(Reporter)

Updated

7 years ago
tracking-fennec: 2.0+ → 2.0b3+
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+
Attachment #486748 - Flags: review?(ben) → review+
Whiteboard: [fennec-checkin-postb2][has-patch]
* 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
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
(Reporter)

Updated

7 years ago
Whiteboard: [fennec-checkin-postb2][has-patch]
Depends on: 611220
This seems to have broken pinch zoom: bug 611220
OS: Mac OS X → All
Hardware: x86 → All
Backed out until the regression is fixed:
http://hg.mozilla.org/mobile-browser/rev/3b0a25f0aab2
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Created attachment 489735 [details] [diff] [review]
fix pinch zoom regression

I think this is the right fix for the regression.
Attachment #489735 - Flags: review?(wjohnston)
Attachment #489735 - Flags: review?(mark.finkle)
Comment on attachment 489735 [details] [diff] [review]
fix pinch zoom regression

Talked on IRC. The changes make sense.
Attachment #489735 - Flags: review?(mark.finkle) → review+
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
Attachment #489735 - Flags: review?(wjohnston)
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
Status: REOPENED → RESOLVED
Last Resolved: 7 years ago7 years ago
Resolution: --- → FIXED
(Reporter)

Updated

7 years ago
Depends on: 611303
Flags: in-testsuite?

Comment 33

7 years ago
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)
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.