The default bug view has changed. See this FAQ.

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

VERIFIED FIXED in Firefox 9

Status

Fennec Graveyard
General
P2
normal
VERIFIED FIXED
6 years ago
3 years ago

People

(Reporter: mbrubeck, Assigned: wesj)

Tracking

(Blocks: 1 bug)

Firefox 9
Firefox 9
Dependency tree / graph

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

6 years ago
The arrowbox popup when a new tab is opened does not currently appear when the tablet layout (bug 656329) is active.
Priority: -- → P2

Updated

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

Comment 3

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

Comment 4

6 years ago
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.
Assignee: lucasr.at.mozilla → wjohnston
(Assignee)

Comment 5

6 years ago
Created attachment 562124 [details]
Screenshot
(In reply to Wesley Johnston (:wesj) from comment #4)
> Created attachment 562123 [details] [diff] [review] [diff] [details] [review]
> Patch v1

Patch is empty?
(Assignee)

Comment 7

6 years ago
Created attachment 562589 [details] [diff] [review]
Patch v1

Ok. What about this. Screenshot coming
Attachment #562123 - Attachment is obsolete: true
Attachment #562589 - Flags: review?(mbrubeck)
(Assignee)

Comment 8

6 years ago
Created attachment 562591 [details]
Screenshot
Attachment #562124 - Attachment is obsolete: true
(Reporter)

Comment 9

6 years ago
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+
(Reporter)

Updated

6 years ago
tracking-fennec: --- → ?
status-firefox9: --- → affected
Keywords: uiwanted
Version: Trunk → Firefox 9
(Assignee)

Comment 10

6 years ago
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.
Attachment #562589 - Attachment is obsolete: true
Attachment #562620 - Flags: review+
(Reporter)

Comment 11

6 years ago
https://hg.mozilla.org/mozilla-central/rev/1f800c226837
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 9
(Reporter)

Updated

6 years ago
status-firefox9: affected → fixed
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.
(Assignee)

Comment 13

6 years ago
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.