Closed Bug 615476 Opened 9 years ago Closed 1 year ago

Arrowpanel arrows should be inset more

Categories

(Toolkit :: Themes, defect)

defect
Not set

Tracking

()

RESOLVED WONTFIX

People

(Reporter: faaborg, Unassigned)

References

Details

(Whiteboard: [target-betaN])

Attachments

(3 files)

Currently the arrow on arrow panels nearly touches the edge.  We should inset this more (about 28 pixels, or whatever the width is of the current arrow).  Also for notification arrow panels we should make sure that the 64x64 icon is centered to the peak of the arrow.
Product: Firefox → Toolkit
QA Contact: general → general
Attached patch patchSplinter Review
This patch insets the arrow more when it is on the top/bottom for all arrow panels.

I based the margin values on centering the doorhanger icon under the arrow. Alex, do we want this much of an inset for all arrow panels, or should I make these margins doorhanger-specific? Also, do we also want the arrow to be inset more when it's on the left/right?
Assignee: nobody → margaret.leibovic
Status: NEW → ASSIGNED
Attachment #496580 - Flags: feedback?(enndeakin)
Centering the arrow under the small icon and the large icon under the arrow is impossible because the icons have even widths and the arrow ends in a 1px point. I opted for pointing the arrow to the left side of center of the small icon, and I aligned the arrow so that the large icon is centered under the small icon (at least that's what I think I did -- I may have been staring at pixels for too long and made a mistake).
Attachment #496583 - Flags: ui-review?(faaborg)
Depends on: 607252
Component: General → Themes
QA Contact: general → themes
Comment on attachment 496580 [details] [diff] [review]
patch

Will this patch move the panel content off-screen when the anchor is near the screen edge?
(In reply to comment #4)
> Comment on attachment 496580 [details] [diff] [review]
> patch
> 
> Will this patch move the panel content off-screen when the anchor is near the
> screen edge?

Yes, that's what it's doing. However, I'm getting a "Popup is offscreen" assertion from my debug build that I need to ignore before the panel will render. Is that okay?
I don't know about the assertion, but from a user point of view this doesn't seem okay...
How often will the popup open that close to the edge of the screen? In that case, browser content is probably off-screen as well, so would the user really be upset by off-screen popup content?

Also, this already happens, so seems like this patch just makes an existing issue more noticeable. And I don't know if there is a good solution for the problem, since the anchor itself can even be off-screen.
No part of the window needs to be off-screen for that. Just make the url bar the first item in the nav bar, e.g. by moving back/forward to the right or up to the tab bar.
Attachment #496583 - Flags: ui-review?(faaborg) → ui-review+
Alex, what do you think about this off-screen issue. I agree with Dão that it would be bad to have the panel appear off-screen if the window is completely on-screen, especially if it is maximized. I would suggest we move the arrow over in that case, but that will make this patch more complicated.
Moving the arrow in the case of being really close to a screen edge is fine (if moving it allows it to fit).  But if there is plenty of room, we should go for the more visually appealing arrow placement.
(In reply to comment #5)
> Yes, that's what it's doing. However, I'm getting a "Popup is offscreen"
> assertion from my debug build that I need to ignore before the panel will
> render. Is that okay?

No, it isn't. That's bug 524545, which I will look into fixing.
I updated the patch in that bug. Could you see if it fixes the issue for you?
Whiteboard: [target-betaN]
Enn's patch prevents the panel from appearing off screen, but the arrow points to the wrong place if the panel is at the edge of the screen, and if the anchor is completely off screen, the arrow disappears.

I'll need to change this patch to have the arrow move over as the anchor gets closer to the edge of the screen, but at some point it won't be able to point to center of the anchor even when the anchor is still on screen, due to the width of the arrow. However, I think this is small enough of an edge case that it's not a big deal.
Comment on attachment 496580 [details] [diff] [review]
patch

On Windows XP (non-aero) the difference that you adjust the panel margin and the arrow margin is one pixel different. On other platforms you adjust both by the same amount. Was this intentional?
Attachment #496580 - Flags: feedback?(enndeakin) → feedback+
Yeah, I did that intentionally to get the arrow to align with the pixel to the left of the center of the anchor icon. Right now the arrows on XP point to the right side of center of the anchor icon, which is different than aero and OSX.
(In reply to comment #13)
> I'll need to change this patch to have the arrow move over as the anchor gets
> closer to the edge of the screen, but at some point it won't be able to point
> to center of the anchor even when the anchor is still on screen, due to the
> width of the arrow. However, I think this is small enough of an edge case that
> it's not a big deal.

It's a big relatively deal as you're affecting arrow panels in general here (e.g. those used for form validation, or those that widgets in the add-on bar might want to use), so it's going to be quite easy to trigger these situations.
Well, that's a case that exists right now, so basically I would be moving the arrow over if possible, and defaulting to the current positioning if there isn't room to move it over.

It's still a problem that currently exists, but I think the solution is out of the scope of this bug. Fixing it would require us to figure out what to do if the anchor icon is at the edge of the screen, and I don't know what that would look like. Maybe something like putting the arrow at the corner of the popup. However, I think that's probably out of the scope of Firefox 4.
Blocks: 767321
Assignee: margaret.leibovic → nobody
Status: ASSIGNED → NEW
This would be a first good move towards the Australis arrow panels.
Depends on: 874792
We now move the arrow when the panel has flip="slide" so the issue that Margaret was running in to (comment #16) may not be an issue anymore.
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #19)
> We now move the arrow when the panel has flip="slide" so the issue that
> Margaret was running in to (comment #16) may not be an issue anymore.

Isn't it only the fact for the menu panel ?
Can this be closed ?
Flags: needinfo?(dao+bmo)
Status: NEW → RESOLVED
Closed: 1 year ago
Flags: needinfo?(dao+bmo)
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.