Last Comment Bug 554937 - Arrow panels
: Arrow panels
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: XP Toolkit/Widgets: XUL (show other bugs)
: Trunk
: All All
: -- normal with 3 votes (vote)
: mozilla2.0b7
Assigned To: Neil Deakin (away until Oct 3)
:
Mentors:
: 599342 602827 (view as bug list)
Depends on: 679302 558072 590073 599342 604464 605332 606343 606606 607252 613843 616502 616607 619669 625413
Blocks: 601719 doorhanger jetpack-panel-apps DownloadsPanel 577927 590105 595432 597557 601789 604257 611231 617661
  Show dependency treegraph
 
Reported: 2010-03-25 07:58 PDT by Neil Deakin (away until Oct 3)
Modified: 2013-11-12 00:57 PST (History)
51 users (show)
highmind63: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
betaN+


Attachments
work in progress (37.84 KB, patch)
2010-07-28 11:08 PDT, Neil Deakin (away until Oct 3)
no flags Details | Diff | Splinter Review
Image of the geolocation arrow panel appearing upwards and to the left (39.29 KB, image/png)
2010-07-28 11:11 PDT, Neil Deakin (away until Oct 3)
no flags Details
Part 1: add an anchorNode property for popups which returns the node the popup is anchored to (4.48 KB, patch)
2010-08-22 10:01 PDT, Neil Deakin (away until Oct 3)
neil: review+
Details | Diff | Splinter Review
Part 2: add an edge attribute to anchor the node on the inside edge (16.44 KB, patch)
2010-08-22 10:05 PDT, Neil Deakin (away until Oct 3)
no flags Details | Diff | Splinter Review
Part 3: arrow popup widget (17.21 KB, patch)
2010-08-22 10:08 PDT, Neil Deakin (away until Oct 3)
no flags Details | Diff | Splinter Review
Part 4: tests (9.31 KB, patch)
2010-08-22 10:09 PDT, Neil Deakin (away until Oct 3)
no flags Details | Diff | Splinter Review
Part 5: support arrows on notification popup (2.92 KB, patch)
2010-08-22 10:10 PDT, Neil Deakin (away until Oct 3)
no flags Details | Diff | Splinter Review
Part 2, version 2: rename the attribute and add clearer comments (16.75 KB, patch)
2010-08-23 07:30 PDT, Neil Deakin (away until Oct 3)
no flags Details | Diff | Splinter Review
Part 2, version 2.1: rename the attribute and add clearer comments (16.75 KB, patch)
2010-08-23 07:49 PDT, Neil Deakin (away until Oct 3)
roc: review+
Details | Diff | Splinter Review
Part 3: arrow popup widget, update to flip attribute (17.20 KB, patch)
2010-08-23 07:49 PDT, Neil Deakin (away until Oct 3)
no flags Details | Diff | Splinter Review
Part 4: tests, with additional checks (10.55 KB, patch)
2010-08-23 08:31 PDT, Neil Deakin (away until Oct 3)
no flags Details | Diff | Splinter Review
Part 1: add an anchorNode property for popups which returns the node the popup is anchored to, address comments (4.56 KB, patch)
2010-09-13 12:34 PDT, Neil Deakin (away until Oct 3)
enndeakin: review+
Details | Diff | Splinter Review
Part 3: arrow popup widget, update to flip attribute, improved styling (35.61 KB, patch)
2010-09-13 13:07 PDT, Neil Deakin (away until Oct 3)
neil: review+
Details | Diff | Splinter Review
image of arrows on each platform (28.39 KB, image/png)
2010-09-13 13:26 PDT, Neil Deakin (away until Oct 3)
no flags Details
Part 3: arrow popup widget, add inherits attribute (35.65 KB, patch)
2010-09-16 18:12 PDT, Neil Deakin (away until Oct 3)
enndeakin: review+
Details | Diff | Splinter Review
Part 6: minor adjustments to theme as brought up by Stephen Horlander (5.49 KB, patch)
2010-09-16 18:18 PDT, Neil Deakin (away until Oct 3)
shorlander: ui‑review+
Details | Diff | Splinter Review
Part 6 Quirks (80.02 KB, image/png)
2010-09-16 19:08 PDT, Stephen Horlander [:shorlander]
no flags Details
Part 5: browser changes, update browser-aero.css as well (4.22 KB, patch)
2010-09-17 11:51 PDT, Neil Deakin (away until Oct 3)
gavin.sharp: review+
Details | Diff | Splinter Review
Part 6: additional theme changes/polish (6.17 KB, patch)
2010-09-17 11:52 PDT, Neil Deakin (away until Oct 3)
gavin.sharp: review+
Details | Diff | Splinter Review
Parts 2 - 6 combined (67.15 KB, patch)
2010-09-23 13:31 PDT, Neil Deakin (away until Oct 3)
no flags Details | Diff | Splinter Review

Description Neil Deakin (away until Oct 3) 2010-03-25 07:58:50 PDT
Support a generic panel subtype with an arrow to its anchor. Involves:

- implementing a binding for this special type of panel.
- adding some event handling when the panel is positioned and sized but before it is visible so that the binding can update the arrow position as needed
Comment 1 Patrick Walton (:pcwalton) 2010-07-21 18:31:41 PDT
Is this still needed for the Console UI? If not should it be removed as a blocker for bug 559481?
Comment 2 Mounir Lamouri (:mounir) 2010-07-26 17:53:45 PDT
We would need this for bug 561636 which should be a blocker for ff4 so I'm asking this bug to be a blocker for 2.0 too.

Neil, do you think that could be done? (if you are in charge of the bug)
Comment 3 Neil Deakin (away until Oct 3) 2010-07-26 18:24:37 PDT
I plan on finishing this soon once the dependency is done. It isn't very difficult and would only take a couple of days to polish off.

However, I don't have the ability to do the appearance/style related work necessary.
Comment 4 Neil Deakin (away until Oct 3) 2010-07-28 11:08:59 PDT
Created attachment 460925 [details] [diff] [review]
work in progress

This patch implements:

<panel type="arrow">

It creates an arrow on a panel which points to its anchor. Several things are included in this patch:

- the arrow reverses/inverts itself when the window is at the edge of the screen
- popup.anchorNode property to return the anchor
- fade="slow|fast|none" to have a slow or fast fade and hide after a few seconds of the popup appearing
- adds position="after_start inside" to made the popup flip on the inside edge of anchor. I will likely use a separate attribute though.
- minor changes to handle this for the existing PopupNotification popups. I tested geolocation and it seems to work ok.
 
Still needs some polish and some real arrow images.
 
Requires the patch for bug 558072.
Comment 5 Neil Deakin (away until Oct 3) 2010-07-28 11:11:20 PDT
Created attachment 460927 [details]
Image of the geolocation arrow panel appearing upwards and to the left
Comment 6 Justin Dolske [:Dolske] 2010-08-18 22:30:15 PDT
Enn: any updates on this? Can this make Beta 5?
Comment 7 Neil Deakin (away until Oct 3) 2010-08-19 16:10:50 PDT
There's nothing more I can do on this bug. As indicated above, I don't know what appearance/style is needed nor do I likely have the ability to do the appearance/style related work necessary.
Comment 8 Stephen Horlander [:shorlander] 2010-08-19 16:16:40 PDT
(In reply to comment #7)
> There's nothing more I can do on this bug. As indicated above, I don't know
> what appearance/style is needed nor do I likely have the ability to do the
> appearance/style related work necessary.

Faaborg filed bug 577927 to handle the actual styling.
Comment 9 Stephen Horlander [:shorlander] 2010-08-19 16:17:56 PDT
(In reply to comment #8)
> (In reply to comment #7)
> > There's nothing more I can do on this bug. As indicated above, I don't know
> > what appearance/style is needed nor do I likely have the ability to do the
> > appearance/style related work necessary.
> 
> Faaborg filed bug 577927 to handle the actual styling.

Actually Peter Henkel did :) Anyway there is a separate bug for styling the arrow panels.
Comment 10 Peter Retzer (:pretzer) 2010-08-20 00:01:41 PDT
(In reply to comment #7)
> There's nothing more I can do on this bug. 

Does that mean you can request review an land it then?
Comment 11 Neil Deakin (away until Oct 3) 2010-08-22 10:01:08 PDT
Created attachment 468151 [details] [diff] [review]
Part 1: add an anchorNode property for popups which returns the node the popup is anchored to
Comment 12 Neil Deakin (away until Oct 3) 2010-08-22 10:05:59 PDT
Created attachment 468152 [details] [diff] [review]
Part 2: add an edge attribute to anchor the node on the inside edge

I 'll have to look at the code again to know why I called it 'edge', or how it flips exactly.
Comment 13 Neil Deakin (away until Oct 3) 2010-08-22 10:08:40 PDT
Created attachment 468153 [details] [diff] [review]
Part 3: arrow popup widget

Includes some images I drew myself or copied from the existing Mac arrow that need to be replaced when the style is polished in bug 577927.
Comment 14 Neil Deakin (away until Oct 3) 2010-08-22 10:09:28 PDT
Created attachment 468154 [details] [diff] [review]
Part 4: tests
Comment 15 Neil Deakin (away until Oct 3) 2010-08-22 10:10:49 PDT
Created attachment 468155 [details] [diff] [review]
Part 5: support arrows on notification popup

Assumes the sample styling I have provided, but will need more polish.
Comment 16 Neil Deakin (away until Oct 3) 2010-08-23 07:30:26 PDT
Created attachment 468306 [details] [diff] [review]
Part 2, version 2: rename the attribute and add clearer comments
Comment 17 Neil Deakin (away until Oct 3) 2010-08-23 07:49:49 PDT
Created attachment 468308 [details] [diff] [review]
Part 2, version 2.1: rename the attribute and add clearer comments

Fix the typo in nsGkAtomList.cpp
Comment 18 Neil Deakin (away until Oct 3) 2010-08-23 07:49:58 PDT
Created attachment 468309 [details] [diff] [review]
Part 3: arrow popup widget, update to flip attribute
Comment 19 Neil Deakin (away until Oct 3) 2010-08-23 08:31:36 PDT
Created attachment 468317 [details] [diff] [review]
Part 4: tests, with additional checks
Comment 20 neil@parkwaycc.co.uk 2010-08-23 08:33:12 PDT
Comment on attachment 468151 [details] [diff] [review]
Part 1: add an anchorNode property for popups which returns the node the popup is anchored to

>-[scriptable, uuid(e4c3845b-97d2-4fdf-860e-949746d15fb9)]
>+[scriptable, uuid(548A9E3F-AF78-42B0-A260-035ECE15C19F)]
Nit: lower case please?

>+  nsIContent* GetAnchor() const { return mAnchorContent.get(); }
Nit: .get() unnecessary.
Comment 21 :Gavin Sharp [email: gavin@gavinsharp.com] 2010-08-26 16:24:45 PDT
Comment on attachment 468155 [details] [diff] [review]
Part 5: support arrows on notification popup

With all of these patches applied (1-5), if I start Firefox with a geolocation-requesting page as the selected tab, I get this:

http://grab.by/667r

Dismissing and re-showing the notification (e.g. by switching tabs) fixes it.
Comment 22 Neil Deakin (away until Oct 3) 2010-08-31 11:56:59 PDT
Gavin, does it work if layout is flushed just before opening the popup?

(Add anchorElement.getBoundingClientRect() or somesuch in PopupNotifications.jsm)
Comment 23 neil@parkwaycc.co.uk 2010-09-01 05:34:29 PDT
Comment on attachment 468309 [details] [diff] [review]
Part 3: arrow popup widget, update to flip attribute

>+        var anchor = this.anchorNode;
>+        if (!anchor) {
>+          arrow.hidden = hideAnchor;
Did you mean true?

>+            arrowbox.dir = popupRect.left + popupRect.width / 2 < anchorRect.left ? "reverse" : "";
[Instead of a spacer and dir="reverse" you could possibly adjust the pack instead to get the same effect. I wasn't sure whether it was worth trying to support centre or arbitrary positioning of the arrow though.]

>+        var fade = getAttribute("fade");
Nit: please use this.getAttribute

>+        var fadeDelay = (fade == "fast") ? 1 : (fade == "slow" ? 4000 : 0);
Nit: don't need ()s, ?: has very low precedence and is right associative.

>+          var self = this;
>+          this._fadeTimer = setTimeout(function () {
>+            self.style.opacity = 0.2;
>+          }, fadeDelay);
[Could also use this:]
this._fadeTimer = setTimeout(function(self) {
  self.style.opacity = 0.2;
}, fadeDelay, this);
Comment 24 :Gavin Sharp [email: gavin@gavinsharp.com] 2010-09-01 11:56:45 PDT
(In reply to comment #22)
> (Add anchorElement.getBoundingClientRect() or somesuch in
> PopupNotifications.jsm)

Doesn't make a difference.
Comment 25 neil@parkwaycc.co.uk 2010-09-01 16:02:47 PDT
I went to all the trouble of building Firefox to try these patches out but I didn't see any arrows :-(
Comment 26 neil@parkwaycc.co.uk 2010-09-12 06:11:31 PDT
Still no joy. Can you perhaps make a tryserver build?
Comment 27 Neil Deakin (away until Oct 3) 2010-09-13 12:34:10 PDT
Created attachment 474773 [details] [diff] [review]
Part 1: add an anchorNode property for popups which returns the node the popup is anchored to, address comments
Comment 28 Neil Deakin (away until Oct 3) 2010-09-13 13:07:45 PDT
Created attachment 474788 [details] [diff] [review]
Part 3: arrow popup widget, update to flip attribute, improved styling

Hopefully, this will work better
Comment 29 Neil Deakin (away until Oct 3) 2010-09-13 13:26:29 PDT
Created attachment 474799 [details]
image of arrows on each platform
Comment 30 Mounir Lamouri (:mounir) 2010-09-13 13:40:31 PDT
I really wonder how that might look on GTK. Do you have a build so I can test? (or even better, you can attach a screenshot ^^)
Comment 31 Neil Deakin (away until Oct 3) 2010-09-13 13:41:21 PDT
The transparency isn't working on GTK correctly, so arrows are disabled there.
Comment 32 neil@parkwaycc.co.uk 2010-09-15 03:21:56 PDT
(In reply to comment #28)
> Created attachment 474788 [details] [diff] [review]
> Hopefully, this will work better
Ah yes, the only minor nit I had here was the async arrow placement.

(In reply to comment #29)
> Created attachment 474799 [details]
> image of arrows on each platform
But strangely I got the Vista look on Server 2008 which doesn't do Aero.
Comment 33 neil@parkwaycc.co.uk 2010-09-15 04:08:21 PDT
Comment on attachment 474788 [details] [diff] [review]
Part 3: arrow popup widget, update to flip attribute, improved styling

>+  <binding id="arrowpanel" extends="chrome://global/content/bindings/popup.xml#panel">
>+    <content flip="both">
>+      <xul:box anonid="container" class="panel-arrowcontainer">
>+        <xul:box anonid="arrowbox" class="panel-arrowbox">
>+          <xul:image anonid="arrow" class="panel-arrow"/>
>+        </xul:box>
>+        <xul:box class="panel-arrowcontent">
>+          <xul:box class="panel-inner-arrowcontent">
[Thought: Are consumers going to want <panel type="arrow" orient="vertical">?
 Or perhaps other attributes, such as align, dir and pack?]

>+        var horizPos = (Math.round(popupRect.right) <= Math.round(anchorRect.left)) ? -1 :
>+                       (Math.round(popupRect.left) >= Math.round(anchorRect.right)) ? 1 : 0;
>+        var vertPos = (Math.round(popupRect.bottom) <= Math.round(anchorRect.top)) ? -1 :
>+                      (Math.round(popupRect.top) >= Math.round(anchorRect.bottom)) ? 1 : 0;
[I'm surprised this was necessary. Or perhaps it wasn't, and something unrelated was stopping arrow panels from working for me.]

>+        var fadeDelay = (fade == "fast") ? 1 : fade == "slow" ? 4000 : 0;
[Could get rid of some more ()s here and above.]
Comment 34 Neil Deakin (away until Oct 3) 2010-09-15 05:34:21 PDT
> [Thought: Are consumers going to want <panel type="arrow" orient="vertical">?
>  Or perhaps other attributes, such as align, dir and pack?]

I will add inherits here.


> >+        var fadeDelay = (fade == "fast") ? 1 : fade == "slow" ? 4000 : 0;
> [Could get rid of some more ()s here and above.]

I find it hard to read without the parentheses.
Comment 35 Neil Deakin (away until Oct 3) 2010-09-16 18:12:58 PDT
Created attachment 476138 [details] [diff] [review]
Part 3: arrow popup widget, add inherits attribute
Comment 36 Neil Deakin (away until Oct 3) 2010-09-16 18:18:27 PDT
Created attachment 476142 [details] [diff] [review]
Part 6: minor adjustments to theme as brought up by Stephen Horlander

Issues from this image: http://grab.by/6mXx

I can't seem to get the shadow perfect on Windows 7 but this this is closer.

Try builds are at http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/neil@mozilla.com-68eb1323a88d/
Comment 37 Csaba Kozák [:WonderCsabo] 2010-09-16 18:38:54 PDT
The Vista doorhanger looks very odd in attachment 474799 [details] . It's very different than shorlander's mockup.
http://tinyurl.com/36newrl
Comment 38 Neil Deakin (away until Oct 3) 2010-09-16 18:54:48 PDT
(In reply to comment #37)
> The Vista doorhanger looks very odd in attachment 474799 [details].
> It's very different than shorlander's mockup.

The implementation is based on the prototype at http://www.stephenhorlander.com/pages/arrow-panel-specs/panel-specs.html
Comment 39 Csaba Kozák [:WonderCsabo] 2010-09-16 18:58:41 PDT
Thanks. There is still a black line wich separates the triangle from the glass border. That line wont be there, will be?
Comment 40 Stephen Horlander [:shorlander] 2010-09-16 19:07:41 PDT
Comment on attachment 476142 [details] [diff] [review]
Part 6: minor adjustments to theme as brought up by Stephen Horlander

This looks a lot better on Windows 7 and OS X (I haven't tested XP yet)

Just a few things.

On OS X the box-shadow is still getting clipped to whatever the container is around the panel+arrow. I specced the box-shadow around the assumption that the native window shadow would be disabled. So if we use the native shadow then the bottom part of that rule is probably not needed. 

>                    0 2px 4px rgba(0,0,0,.65);

On Windows 7 the panel is appearing in a tooltip. Also the glass border is 7px instead of 6px makes it feel a little fat.

Otherwise it looks awesome to me with that stuff fixed.
Comment 41 Stephen Horlander [:shorlander] 2010-09-16 19:08:53 PDT
Created attachment 476154 [details]
Part 6 Quirks

Screenshot of problems mentioned in review.
Comment 42 Dão Gottwald [:dao] 2010-09-16 23:41:28 PDT
Comment on attachment 476138 [details] [diff] [review]
Part 3: arrow popup widget, add inherits attribute

>--- a/toolkit/themes/winstripe/global/popup.css
>+++ b/toolkit/themes/winstripe/global/popup.css

>+.panel-arrowcontent {
>+  -moz-border-radius: 6px;
>+  background: -moz-linear-gradient(rgba(248,250,253,1), rgba(248,250,253,1) 20px, rgba(248,250,253,.97));
>+  padding: 6px;
>+  margin: 1px;
>+  -moz-box-shadow: 0 0 5px 1px rgba(184,205,232,1) inset,
>+                   0 0 0 1px rgba(0,0,0,.25),
>+                   0 1px 5px rgba(0,0,0,.5);
>+}

This seems to specify an almost completely opaque background color without a corresponding foreground color, which would fail when the default foreground color isn't dark.

-moz- has been dropped from border-radius and box-shadow.
Comment 43 Luke Iliffe (Harlequin99) 2010-09-17 01:40:17 PDT
Will this bug apply arrows to doorhanger notifications only or will it apply to other panels, e.g. the edit bookmark panel? Arrows for this were hinted at in bug 413059. 

If this is doorhanger notifications only is it worth filing a followup for other panels?
Comment 44 Robert Kaiser 2010-09-17 05:24:46 PDT
(In reply to comment #43)
> Will this bug apply arrows to doorhanger notifications only or will it apply to
> other panels, e.g. the edit bookmark panel?

From all I've seen, this creates the infrastructure for making any panel an arrow panel, and applies it to doorhangers.

> If this is doorhanger notifications only is it worth filing a followup for
> other panels?

I think it might be worth filing followups for other panels where this makes sense. FWIW, there is already a bug on file for converting the edit bookmarks panel this way in SeaMonkey, I'd love to see Firefox doing the same. :)
Comment 45 Neil Deakin (away until Oct 3) 2010-09-17 11:51:12 PDT
Created attachment 476325 [details] [diff] [review]
Part 5: browser changes, update browser-aero.css as well
Comment 46 Neil Deakin (away until Oct 3) 2010-09-17 11:52:12 PDT
Created attachment 476327 [details] [diff] [review]
Part 6: additional theme changes/polish
Comment 47 Alex Faaborg [:faaborg] (Firefox UX) 2010-09-17 15:14:28 PDT
> If this is doorhanger notifications only is it worth filing a followup for
> other panels?

Yep, for Firefox:

Bug 597557 - Bookmarks panel should use an Arrowpanel
Bug 597556 - Site Identity info should use an Arrowpanel
Comment 48 neil@parkwaycc.co.uk 2010-09-17 17:24:17 PDT
(In reply to comment #34)
>>>+        var fadeDelay = (fade == "fast") ? 1 : fade == "slow" ? 4000 : 0;
>>[Could get rid of some more ()s here and above.]
>I find it hard to read without the parentheses.
In your original patch you actually wrote (fade == "slow" ? 4000 : 0); perhaps you really meant to write (fade == "slow") ? 4000 : 0 instead?
Comment 49 Neil Deakin (away until Oct 3) 2010-09-21 11:02:17 PDT
Checked in the first part: http://hg.mozilla.org/mozilla-central/rev/c4d43faf3616
Comment 50 Neil Deakin (away until Oct 3) 2010-09-23 13:31:17 PDT
Created attachment 478036 [details] [diff] [review]
Parts 2 - 6 combined
Comment 51 :Gavin Sharp [email: gavin@gavinsharp.com] 2010-09-23 16:32:11 PDT
Comment on attachment 476325 [details] [diff] [review]
Part 5: browser changes, update browser-aero.css as well

I'm seeing alignment issues on Windows: http://grab.by/6x6Z
Looks good on Mac, though it's a bit more compact (think we probably want some extra padding). Are there going to be a followups for styling tweaks?
Comment 52 Alex Limi (:limi) — Firefox UX Team 2010-09-24 12:51:31 PDT
(In reply to comment #51)
> I'm seeing alignment issues on Windows: http://grab.by/6x6Z
> Looks good on Mac, though it's a bit more compact (think we probably want some
> extra padding). Are there going to be a followups for styling tweaks?

Yeah, styling tweaks should be OK to do post-b7 in polish follow-up bugs (unless they change APIs etc, which I assume they won't :)
Comment 53 imradyurrad 2010-09-30 12:51:03 PDT
So is anybody going to check this in?
Comment 54 Josh Tumath 2010-09-30 12:55:41 PDT
(In reply to comment #53)
> So is anybody going to check this in?

If I recall correctly, m-c is frozen, and is accepting only beta 7 blockers and crash bugs. This will land once it opens again.
Comment 55 :Margaret Leibovic 2010-09-30 15:02:27 PDT
*** Bug 599342 has been marked as a duplicate of this bug. ***
Comment 56 Vova Olar 2010-10-04 13:34:40 PDT
Could someone add Bug 600800 to "Blocks" list? Thanks.
Comment 57 Frank Yan (:fryn) 2010-10-10 09:01:45 PDT
*** Bug 602827 has been marked as a duplicate of this bug. ***
Comment 58 Nochum Sossonko [:Natch] 2010-10-13 12:46:08 PDT
Pushed to m-c: http://hg.mozilla.org/mozilla-central/rev/675c7c026824

I missed some reviewers in the commit, roc, neil, shorlander (ui-r)...
Comment 59 :Margaret Leibovic 2010-10-14 11:42:50 PDT
(In reply to comment #51)
> Comment on attachment 476325 [details] [diff] [review]
> Part 5: browser changes, update browser-aero.css as well
> 
> I'm seeing alignment issues on Windows: http://grab.by/6x6Z
> Looks good on Mac, though it's a bit more compact (think we probably want some
> extra padding). Are there going to be a followups for styling tweaks?

I'm going to fix these issues in the patches for the doorhanger styling bugs (tracked in bug 577927).
Comment 60 Nochum Sossonko [:Natch] 2010-10-14 18:31:35 PDT
Comment on attachment 474773 [details] [diff] [review]
Part 1: add an anchorNode property for popups which returns the node the popup is anchored to, address comments

Any chance we can get these patches explicitly approved for beta 7, so the feature gets more exposure?

Also, this is kind of a new feature (introducing a new XUL widget), so if beta 7 is feature freeze it really should land there...
Comment 61 Eric Shepherd [:sheppy] 2010-11-04 12:18:47 PDT
I've started updating documentation, but have questions:

What exactly does "flip" do? As far as I can tell it has no effect whatsoever in my tests, so clearly there's something I'm missing here. Also, "fade" doesn't seem to have any effect, so again, I must be missing something.

Documents updated so far:

https://developer.mozilla.org/en/XUL/Property/anchorNode (new page)
https://developer.mozilla.org/en/XUL/Attribute/panel.type (new page)

https://developer.mozilla.org/en/XUL/menupopup
https://developer.mozilla.org/en/XUL/panel
Comment 62 Neil Deakin (away until Oct 3) 2010-11-04 12:42:14 PDT
The fade attribute may be used to have the arrow popup fade away a few moments after it appears. fade="slow|fast|none" (none is the default) to have a slow or fast fade.

Normally, when a popup is opened near the edge of the screen, it flips over to open on the opposite side of the anchor. For example, a menu with not enough room to open downwards will open upwards instead. That's the vertical flip. (Essentially, flipped vertically on the opposite side of the anchor). Horizontally, the menu won't flip, and will instead just be moved onscreen. That makes sense, as, for example, a File menu appearing opening to the left would look unusual.

For arrow popups, doing horizontal flipping as well as vertical is more suitable. The flip attribute controls this behaviour (flip="both" may be used to flip around the anchor in both directions, instead of just the one direction). However, note that the default value is "both" for arrow popups so generally, the flip attribute wouldn't be used directly.
Comment 63 Eric Shepherd [:sheppy] 2010-11-04 13:27:02 PDT
That's just what I was looking for; explains why adding flip="both" to my arrow popup was having no effect, and why I kept clicking away at my button and never seeing a fade. :)

Added these:

https://developer.mozilla.org/en/XUL/Attribute/panel.fade
https://developer.mozilla.org/en/XUL/Attribute/panel.flip

And those are listed on https://developer.mozilla.org/en/XUL/panel now, as well as on Fx4 for developers.

Please have a look at these and make sure I got the details right. Thanks!
Comment 64 Nochum Sossonko [:Natch] 2010-11-04 15:16:07 PDT
Can you please also document the position restrictions on arrow panels? There must be at least 1 pixel between the anchorNode and the panel on the side or top/bottom, otherwise the arrow doesn't show. That's a potential pitfall for developers...
Comment 65 Eric Shepherd [:sheppy] 2010-11-05 05:44:34 PDT
Updated per comment 64:

https://developer.mozilla.org/en/XUL/Attribute/panel.type
Comment 66 Neil Deakin (away until Oct 3) 2010-11-05 06:18:08 PDT
(In reply to comment #64)
> Can you please also document the position restrictions on arrow panels? There
> must be at least 1 pixel between the anchorNode and the panel on the side or
> top/bottom, otherwise the arrow doesn't show.

Are you saying that the arrow doesn't appear if there is no space between the anchor and the panel?

The tests added by the bug don't show that to be the case.

If any documentation should exist, it should indicate that the arrow will appear as long as the panel is on one side of the anchor. If the panel and anchor overlap, the arrow will not appear.
Comment 67 Nochum Sossonko [:Natch] 2010-11-05 10:53:29 PDT
(In reply to comment #66)
> (In reply to comment #64)
> > Can you please also document the position restrictions on arrow panels? There
> > must be at least 1 pixel between the anchorNode and the panel on the side or
> > top/bottom, otherwise the arrow doesn't show.
> 
> Are you saying that the arrow doesn't appear if there is no space between the
> anchor and the panel?
> 
> The tests added by the bug don't show that to be the case.
> 
> If any documentation should exist, it should indicate that the arrow will
> appear as long as the panel is on one side of the anchor. If the panel and
> anchor overlap, the arrow will not appear.

That's exactly what I meant, the panel and anchor can't overlap. Sorry for the confusion.
Comment 68 gadjo 2010-12-06 23:52:11 PST
@Nochum - please see bug 616502 (has a testcase), there is missing arrow
problem, but I don't think that panel and anchor are overlapping there

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