The default bug view has changed. See this FAQ.

Status

()

Core
XP Toolkit/Widgets: XUL
RESOLVED FIXED
7 years ago
3 years ago

People

(Reporter: Neil Deakin, Assigned: Neil Deakin)

Tracking

(Depends on: 1 bug, Blocks: 1 bug, {dev-doc-complete})

Trunk
mozilla2.0b7
dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking2.0 betaN+)

Details

Attachments

(10 attachments, 10 obsolete attachments)

39.29 KB, image/png
Details
16.75 KB, patch
roc
: review+
Details | Diff | Splinter Review
10.55 KB, patch
Details | Diff | Splinter Review
4.56 KB, patch
Neil Deakin
: review+
Details | Diff | Splinter Review
28.39 KB, image/png
Details
35.65 KB, patch
Neil Deakin
: review+
Details | Diff | Splinter Review
80.02 KB, image/png
Details
4.22 KB, patch
Gavin
: review+
Details | Diff | Splinter Review
6.17 KB, patch
Gavin
: review+
Details | Diff | Splinter Review
67.15 KB, patch
Details | Diff | Splinter Review
(Assignee)

Description

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

Updated

7 years ago
Depends on: 558072
Blocks: 398776

Updated

7 years ago
Blocks: 529086

Updated

7 years ago
Blocks: 559481

Updated

7 years ago
Blocks: 564934
Blocks: 494238
Blocks: 577927

Updated

7 years ago
No longer blocks: 529086
Is this still needed for the Console UI? If not should it be removed as a blocker for bug 559481?

Updated

7 years ago
No longer blocks: 559481
Blocks: 561636
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)
blocking2.0: --- → ?
(Assignee)

Comment 3

7 years ago
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.
blocking2.0: ? → betaN+
(Assignee)

Comment 4

7 years ago
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.
Assignee: nobody → enndeakin
Status: NEW → ASSIGNED
(Assignee)

Comment 5

7 years ago
Created attachment 460927 [details]
Image of the geolocation arrow panel appearing upwards and to the left
Enn: any updates on this? Can this make Beta 5?
(Assignee)

Comment 7

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

Comment 11

7 years ago
Created attachment 468151 [details] [diff] [review]
Part 1: add an anchorNode property for popups which returns the node the popup is anchored to
Attachment #460925 - Attachment is obsolete: true
Attachment #468151 - Flags: review?(neil)
(Assignee)

Comment 12

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

Comment 13

7 years ago
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.
Attachment #468153 - Flags: review?(neil)
(Assignee)

Comment 14

7 years ago
Created attachment 468154 [details] [diff] [review]
Part 4: tests
(Assignee)

Comment 15

7 years ago
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.
Attachment #468155 - Flags: review?(gavin.sharp)
(Assignee)

Comment 16

7 years ago
Created attachment 468306 [details] [diff] [review]
Part 2, version 2: rename the attribute and add clearer comments
Attachment #468152 - Attachment is obsolete: true
Attachment #468306 - Flags: review?(roc)
(Assignee)

Comment 17

7 years ago
Created attachment 468308 [details] [diff] [review]
Part 2, version 2.1: rename the attribute and add clearer comments

Fix the typo in nsGkAtomList.cpp
Attachment #468306 - Attachment is obsolete: true
Attachment #468308 - Flags: review?(roc)
Attachment #468306 - Flags: review?(roc)
(Assignee)

Comment 18

7 years ago
Created attachment 468309 [details] [diff] [review]
Part 3: arrow popup widget, update to flip attribute
Attachment #468153 - Attachment is obsolete: true
Attachment #468309 - Flags: review?(neil)
Attachment #468153 - Flags: review?(neil)
(Assignee)

Comment 19

7 years ago
Created attachment 468317 [details] [diff] [review]
Part 4: tests, with additional checks
Attachment #468154 - Attachment is obsolete: true
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.
Attachment #468151 - Flags: review?(neil) → review+
Attachment #468308 - Flags: review?(roc) → review+

Updated

7 years ago
Blocks: 590105
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.
(Assignee)

Comment 22

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

(Add anchorElement.getBoundingClientRect() or somesuch in PopupNotifications.jsm)
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);
(In reply to comment #22)
> (Add anchorElement.getBoundingClientRect() or somesuch in
> PopupNotifications.jsm)

Doesn't make a difference.
I went to all the trouble of building Firefox to try these patches out but I didn't see any arrows :-(
Blocks: 595432
No longer blocks: 561636
Still no joy. Can you perhaps make a tryserver build?
(Assignee)

Comment 27

7 years ago
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
Attachment #468151 - Attachment is obsolete: true
Attachment #474773 - Flags: review+
(Assignee)

Comment 28

7 years ago
Created attachment 474788 [details] [diff] [review]
Part 3: arrow popup widget, update to flip attribute, improved styling

Hopefully, this will work better
Attachment #468309 - Attachment is obsolete: true
Attachment #474788 - Flags: review?(neil)
Attachment #468309 - Flags: review?(neil)
(Assignee)

Updated

7 years ago
Attachment #474788 - Flags: review?(gavin.sharp)
(Assignee)

Comment 29

7 years ago
Created attachment 474799 [details]
image of arrows on each platform
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 ^^)
(Assignee)

Comment 31

7 years ago
The transparency isn't working on GTK correctly, so arrows are disabled there.
(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 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.]
Attachment #474788 - Flags: review?(neil) → review+
(Assignee)

Comment 34

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

Updated

7 years ago
Attachment #474788 - Flags: review?(gavin.sharp)
(Assignee)

Comment 35

7 years ago
Created attachment 476138 [details] [diff] [review]
Part 3: arrow popup widget, add inherits attribute
Attachment #474788 - Attachment is obsolete: true
Attachment #476138 - Flags: review+
(Assignee)

Comment 36

7 years ago
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/
Attachment #476142 - Flags: ui-review?(shorlander)
The Vista doorhanger looks very odd in attachment 474799 [details] . It's very different than shorlander's mockup.
http://tinyurl.com/36newrl
(Assignee)

Comment 38

7 years ago
(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
Thanks. There is still a black line wich separates the triangle from the glass border. That line wont be there, will be?
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.
Attachment #476142 - Flags: ui-review?(shorlander) → ui-review+
Created attachment 476154 [details]
Part 6 Quirks

Screenshot of problems mentioned in review.
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.
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

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

Comment 45

7 years ago
Created attachment 476325 [details] [diff] [review]
Part 5: browser changes, update browser-aero.css as well
Attachment #468155 - Attachment is obsolete: true
Attachment #476325 - Flags: review?(gavin.sharp)
Attachment #468155 - Flags: review?(gavin.sharp)
(Assignee)

Comment 46

7 years ago
Created attachment 476327 [details] [diff] [review]
Part 6: additional theme changes/polish
Attachment #476142 - Attachment is obsolete: true
Attachment #476327 - Flags: review?(gavin.sharp)
> 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
Blocks: 597557, 597556
(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?
(Assignee)

Comment 49

7 years ago
Checked in the first part: http://hg.mozilla.org/mozilla-central/rev/c4d43faf3616
(Assignee)

Comment 50

7 years ago
Created attachment 478036 [details] [diff] [review]
Parts 2 - 6 combined
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?
Attachment #476325 - Flags: review?(gavin.sharp) → review+
Attachment #476327 - Flags: review?(gavin.sharp) → review+
(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 :)
Depends on: 599342
(Assignee)

Updated

7 years ago
Keywords: checkin-needed

Comment 53

7 years ago
So is anybody going to check this in?

Comment 54

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

Updated

7 years ago
Duplicate of this bug: 599342

Updated

7 years ago
Blocks: 601719

Comment 56

7 years ago
Could someone add Bug 600800 to "Blocks" list? Thanks.
Blocks: 601789

Updated

7 years ago
Duplicate of this bug: 602827
Pushed to m-c: http://hg.mozilla.org/mozilla-central/rev/675c7c026824

I missed some reviewers in the commit, roc, neil, shorlander (ui-r)...
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b8

Updated

7 years ago
Blocks: 604257
Keywords: dev-doc-needed

Comment 59

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

Updated

7 years ago
Depends on: 604464
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...
Attachment #474773 - Flags: approval2.0?
Attachment #478036 - Flags: approval2.0?

Updated

7 years ago
Depends on: 605332

Updated

7 years ago
Blocks: 606343
Depends on: 606606

Updated

7 years ago
Depends on: 607252

Updated

7 years ago
No longer blocks: 606343
Depends on: 606343
Target Milestone: mozilla2.0b8 → mozilla2.0b7
Attachment #474773 - Flags: approval2.0?
Attachment #478036 - Flags: approval2.0?
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
(Assignee)

Comment 62

7 years ago
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.
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!
Keywords: dev-doc-needed → dev-doc-complete
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...
Updated per comment 64:

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

Comment 66

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

Updated

7 years ago
No longer blocks: 597556
Blocks: 611231
Depends on: 613843

Updated

6 years ago
Depends on: 616607

Comment 68

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

Updated

6 years ago
Blocks: 617661
Depends on: 616502

Updated

6 years ago
Depends on: 590073

Updated

6 years ago
Depends on: 619669

Updated

6 years ago
Depends on: 625413

Updated

6 years ago
Depends on: 679302
You need to log in before you can comment on or make changes to this bug.