Last Comment Bug 677670 - "New tab opened" popup does not work in tablet mode
: "New tab opened" popup does not work in tablet mode
Status: VERIFIED FIXED
:
Product: Fennec Graveyard
Classification: Graveyard
Component: General (show other bugs)
: Firefox 9
: All All
: P2 normal (vote)
: Firefox 9
Assigned To: Wesley Johnston (:wesj)
:
:
Mentors:
Depends on:
Blocks: 655762 656329
  Show dependency treegraph
 
Reported: 2011-08-09 13:27 PDT by Matt Brubeck (:mbrubeck)
Modified: 2013-12-10 10:00 PST (History)
14 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch v1 (71 bytes, patch)
2011-09-23 12:08 PDT, Wesley Johnston (:wesj)
no flags Details | Diff | Splinter Review
Screenshot (38.19 KB, image/png)
2011-09-23 12:08 PDT, Wesley Johnston (:wesj)
no flags Details
Patch v1 (4.01 KB, patch)
2011-09-26 17:05 PDT, Wesley Johnston (:wesj)
mbrubeck: review+
Details | Diff | Splinter Review
Screenshot (27.07 KB, image/png)
2011-09-26 17:06 PDT, Wesley Johnston (:wesj)
no flags Details
For checkin (4.37 KB, patch)
2011-09-26 18:19 PDT, Wesley Johnston (:wesj)
wjohnston2000: review+
Details | Diff | Splinter Review

Description Matt Brubeck (:mbrubeck) 2011-08-09 13:27:18 PDT
The arrowbox popup when a new tab is opened does not currently appear when the tablet layout (bug 656329) is active.
Comment 1 Lucas Rocha (:lucasr) 2011-09-15 15:38:27 PDT
Madhava, Brian, Ian: how should the "New tab opened" popup show up in tablet mode for each orientation? In portrait, maybe the popup points to the tabs toggle button? Not sure how it would behave in landscape mode though.
Comment 2 Mark Finkle (:mfinkle) (use needinfo?) 2011-09-15 20:19:17 PDT
Maybe we shouldn't use it in landscape mode. In portrait mode, pointing to the tabs toggle sounds good.
Comment 3 Wesley Johnston (:wesj) 2011-09-22 22:51:14 PDT
Just saw this again tonight. Wanted to bump this to remind myself to look at it tomorrow. Killing this entirely in tablet mode seems reasonable to me, but I'll look at having it point at the tabs button in portrait if we want.
Comment 4 Wesley Johnston (:wesj) 2011-09-23 12:08:19 PDT
Created attachment 562123 [details] [diff] [review]
Patch v1

Patch to just not use this an Honeycomb. I'll post a screenshot, but I think our arrowboxes just look kinda ugly right now on tablets. On the other hand, if you select "Open in New Tab" from the context menu, not much really happens in tablet mode (a little number in the upper left goes up by one), which might confuse people.
Comment 5 Wesley Johnston (:wesj) 2011-09-23 12:08:56 PDT
Created attachment 562124 [details]
Screenshot
Comment 6 Lucas Rocha (:lucasr) 2011-09-23 12:38:24 PDT
(In reply to Wesley Johnston (:wesj) from comment #4)
> Created attachment 562123 [details] [diff] [review] [diff] [details] [review]
> Patch v1

Patch is empty?
Comment 7 Wesley Johnston (:wesj) 2011-09-26 17:05:33 PDT
Created attachment 562589 [details] [diff] [review]
Patch v1

Ok. What about this. Screenshot coming
Comment 8 Wesley Johnston (:wesj) 2011-09-26 17:06:38 PDT
Created attachment 562591 [details]
Screenshot
Comment 9 Matt Brubeck (:mbrubeck) 2011-09-26 17:23:24 PDT
Comment on attachment 562589 [details] [diff] [review]
Patch v1

>+++ b/mobile/chrome/content/bindings/arrowbox.xml

>+                  if (this.anchorNode && this.anchorNode.hasAttribute("anchorPosX"))

This breaks the formhelper suggestion arrowbox because "this.anchorNode.hasAttribute is not a function".  (It passes anchorTo a Rect instead of an Element.)  Add an extra guard "&& this.anchorNode.hasAttribute"?

>               } else if (vertPos == 0) {
>                 container.orient = "";
>                 arrowbox.orient = "vertical";
>+                  let anchorPosY = 0.5;
>+                  if (this.anchorNode && this.anchorNode.hasAttribute("anchorPosY"))
>+                     anchorPosY = parseFloat(this.anchorNode.getAttribute("anchorPosY")) || 0.5;
>+                arrowbox.style.marginTop = ((targetRect.top - popupRect.top) + (targetRect.height * anchorPosY) - HALF_ARROW_WIDTH) + "px";

Same here.  Intentation needs adjustment too.

>+++ b/mobile/chrome/content/common-ui.js

>   show: function nt_show(aTab) {
>-    if (Util.isTablet())
>+    if (Util.isTablet() && TabsPopup.visible)
>       return;
> 

Does this depend on another patch?  It didn't apply cleanly in my tree.

Looks good aside from the minor issues above.

We should rememember to get some fix into Firefox 9 on Aurora if this doesn't land in time.
Comment 10 Wesley Johnston (:wesj) 2011-09-26 18:19:57 PDT
Created attachment 562620 [details] [diff] [review]
For checkin

Patch for checkin. I have to head home and can't watch the tree. If someone wants to land, great. Otherwise, I'll try to do it later tonight.
Comment 11 Matt Brubeck (:mbrubeck) 2011-09-26 18:31:26 PDT
https://hg.mozilla.org/mozilla-central/rev/1f800c226837
Comment 12 Mark Finkle (:mfinkle) (use needinfo?) 2011-09-26 19:19:21 PDT
Based on the day I'm having it might not come as a shock that I don't like this patch. "anchorPosX" is not a good idea. I don't like making the anchor nodes carry info about potential arrow boxes.

arrowbox.xml has new complexity as well.

Why not just make a new <arrowbox> and use it for portrait mode on tablets. Get NewTabPopup JS object to just manage the popups. I'd rather add an element than start adding complexity to the existing code.

Heck, just have NewTabPopup _create_ and_destroy_ <arrowbox>s as needed.

Please file a followup bug to clean this up.
Comment 13 Wesley Johnston (:wesj) 2011-09-26 19:38:25 PDT
Sorry. I figured you'd probably glance at this before it went in, then things just went much faster than my normal patch cycle.

The issue here isn't the arrowbox really. The tool-tabs button is actually fairly large. If we just tell the arrow to point at it, it points somewhere between the back button and new tabs button (see the first screenshot I posted here). I tried pointing at the .toolbarbutton-icon or toolbarbutton-text fields as well, but both of that actually flex to also take up more space than they appear to take up.

My theory was, we need some way for anchors that have odd shapes or sizes like this to designate what an arrow box should point at. Alternatively, the NewTabPopup.show method could probably move the arrowbox after it is shown, or pass in a fancy rect like the form helper does? I can put that together tomorrow and get rid of this if you want.
Comment 14 Cristian Nicolae (:xti) 2011-09-27 05:19:52 PDT
Verified fixed on:
Mozilla/5.0 (Android;Linux armv7l;rv:9.0a1)Gecko/20110927
Firefox/9.0a1 Fennec/9.0a1
Device: Acer ICONIA A500
OS: Android 3.1

Note You need to log in before you can comment on or make changes to this bug.