554937 - Arrow panels

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Neil Deakin]

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

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

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]

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]

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

Attachment: Image of the geolocation arrow panel appearing upwards and to the left

Comment 6

[Justin Dolske [:Dolske]]

Enn: any updates on this? Can this make Beta 5?

Comment 7

[Neil Deakin]

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

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

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

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

Attachment: Part 1: add an anchorNode property for popups which returns the node the popup is anchored to

Comment 12

[Neil Deakin]

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

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

Attachment: Part 4: tests

Comment 15

[Neil Deakin]

Attachment: Part 5: support arrows on notification popup

Assumes the sample styling I have provided, but will need more polish.

Comment 16

[Neil Deakin]

Attachment: Part 2, version 2: rename the attribute and add clearer comments

Comment 17

[Neil Deakin]

Attachment: Part 2, version 2.1: rename the attribute and add clearer comments

Fix the typo in nsGkAtomList.cpp

Comment 18

[Neil Deakin]

Attachment: Part 3: arrow popup widget, update to flip attribute

Comment 19

[Neil Deakin]

Attachment: Part 4: tests, with additional checks

Comment 20

[[e-mail]]

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: [e-mail]]]

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]

Gavin, does it work if layout is flushed just before opening the popup?

(Add anchorElement.getBoundingClientRect() or somesuch in PopupNotifications.jsm)

Comment 23

[[e-mail]]

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: [e-mail]]]

(In reply to comment #22)
> (Add anchorElement.getBoundingClientRect() or somesuch in
> PopupNotifications.jsm)

Doesn't make a difference.

Comment 25

[[e-mail]]

I went to all the trouble of building Firefox to try these patches out but I didn't see any arrows :-(

Comment 26

[[e-mail]]

Still no joy. Can you perhaps make a tryserver build?

Comment 27

[Neil Deakin]

Attachment: Part 1: add an anchorNode property for popups which returns the node the popup is anchored to, address comments

Comment 28

[Neil Deakin]

Attachment: Part 3: arrow popup widget, update to flip attribute, improved styling

Hopefully, this will work better

Comment 29

[Neil Deakin]

Attachment: image of arrows on each platform

Comment 30

[Mounir Lamouri (:mounir)]

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]

The transparency isn't working on GTK correctly, so arrows are disabled there.

Comment 32

[[e-mail]]

(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

[[e-mail]]

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]

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

Attachment: Part 3: arrow popup widget, add inherits attribute

Comment 36

[Neil Deakin]

Attachment: 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/[e-mail]-68eb1323a88d/

Comment 37

[Csaba Kozák [:WonderCsabo]]

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]

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

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

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

Attachment: Part 6 Quirks

Screenshot of problems mentioned in review.

Comment 42

[Dão Gottwald [:dao]]

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

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]

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

Attachment: Part 5: browser changes, update browser-aero.css as well

Comment 46

[Neil Deakin]

Attachment: Part 6: additional theme changes/polish

Comment 47

[Alex Faaborg [:faaborg] (Firefox UX)]

> 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

[[e-mail]]

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

Checked in the first part: http://hg.mozilla.org/mozilla-central/rev/c4d43faf3616

Comment 50

[Neil Deakin]

Attachment: Parts 2 - 6 combined

Comment 51

[:Gavin Sharp [email: [e-mail]]]

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]

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

So is anybody going to check this in?

Comment 54

[Josh Tumath]

(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 56

[Vova Olar]

Could someone add Bug 600800 to "Blocks" list? Thanks.

Comment 58

[Nochum Sossonko [:Natch]]

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]

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

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

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]

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

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

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

Updated per comment 64:

https://developer.mozilla.org/en/XUL/Attribute/panel.type

Comment 66

[Neil Deakin]

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

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

@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

Comment 69

[Firefox Bug Husbandry Bot]

Moving to Core:XUL per https://bugzilla.mozilla.org/show_bug.cgi?id=1455336