Closed Bug 677670 Opened 9 years ago Closed 9 years ago

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

Categories

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

Firefox 9
defect

Tracking

(firefox9 fixed)

VERIFIED FIXED
Firefox 9
Tracking Status
firefox9 --- fixed

People

(Reporter: mbrubeck, Assigned: wesj)

References

Details

Attachments

(2 files, 3 obsolete files)

The arrowbox popup when a new tab is opened does not currently appear when the tablet layout (bug 656329) is active.
Priority: -- → P2
Assignee: nobody → lucasr.at.mozilla
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.
Keywords: uiwanted
Maybe we shouldn't use it in landscape mode. In portrait mode, pointing to the tabs toggle sounds good.
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.
Attached patch Patch v1 (obsolete) — Splinter Review
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.
Assignee: lucasr.at.mozilla → wjohnston
Attached image Screenshot (obsolete) —
(In reply to Wesley Johnston (:wesj) from comment #4)
> Created attachment 562123 [details] [diff] [review] [diff] [details] [review]
> Patch v1

Patch is empty?
Attached patch Patch v1 (obsolete) — Splinter Review
Ok. What about this. Screenshot coming
Attachment #562123 - Attachment is obsolete: true
Attachment #562589 - Flags: review?(mbrubeck)
Attached image Screenshot
Attachment #562124 - Attachment is obsolete: true
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.
Attachment #562589 - Flags: review?(mbrubeck) → review+
tracking-fennec: --- → ?
Keywords: uiwanted
Version: Trunk → Firefox 9
Attached patch For checkinSplinter Review
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.
Attachment #562589 - Attachment is obsolete: true
Attachment #562620 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/1f800c226837
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 9
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.
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.
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
Status: RESOLVED → VERIFIED
tracking-fennec: ? → ---
You need to log in before you can comment on or make changes to this bug.