677670 - "New tab opened" popup does not work in tablet mode

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect, P2)

Description

[Matt Brubeck (:mbrubeck)]

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

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?)]

Maybe we shouldn't use it in landscape mode. In portrait mode, pointing to the tabs toggle sounds good.

Comment 3

[Wesley Johnston (:wesj)]

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

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

Attachment: Screenshot

Comment 6

[Lucas Rocha (:lucasr)]

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

Attachment: Patch v1

Ok. What about this. Screenshot coming

Comment 8

[Wesley Johnston (:wesj)]

Attachment: Screenshot

Comment 9

[Matt Brubeck (:mbrubeck)]

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

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

https://hg.mozilla.org/mozilla-central/rev/1f800c226837

Comment 12

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

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

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

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