597557 - Bookmarks & Identity panels should use an Arrowpanel

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

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

Once we have arrowpanels landed over in bug 554937, we should use one for these for the panel produced by the star icon in the location bar.

Comment 1

[:Margaret Leibovic]

Requesting blocking because the edit bookmarks panel looks pretty bad on windows and linux, and this shouldn't be very hard to change.

Comment 2

[Nochum Sossonko [:Natch]]

Attachment: patch

Tentatively taking. This is for the bookmark panel and the site identity icon.

Comment 3

[:Margaret Leibovic]

When investigating bug 604464, I discovered there's some unnecessary css for these panels in the pinstripe theme now. There may be more, but we can at least get rid of http://mxr.mozilla.org/mozilla-central/source/browser/themes/pinstripe/browser/browser.css#1082 and http://mxr.mozilla.org/mozilla-central/source/browser/themes/pinstripe/browser/browser.css#1872 when these turn into arrow panels.

Comment 4

[Nochum Sossonko [:Natch]]

(In reply to comment #3)
Ok. I don't think that conflicts with this bug or the patch provided though.

Btw, I noticed that the arrow panels don't align perfectly with the anchorNode now, since panels align based on the position attribute (or argument passed to openPopup). That doesn't give you the option of aligning with the arrow. Is there a bug for that or is it covered somewhere?

Comment 5

[Neil Deakin]

Comment on attachment 483281 [details] [diff] [review]
patch

>diff --git a/browser/base/content/browser.xul b/browser/base/content/browser.xul
>--- a/browser/base/content/browser.xul
>+++ b/browser/base/content/browser.xul
>@@ -168,9 +168,11 @@
>     <panel id="invalid-form-popup" noautofocus="true" hidden="true" level="parent"/>
> 
>     <panel id="editBookmarkPanel"
>+           type="arrow"
>            orient="vertical"
>            ignorekeys="true"
>            hidden="true"
>+           level="parent"

Is thee are reason you need to set the level here? This will likely break IME usage.


>-    <panel id="identity-popup" position="after_start" hidden="true" noautofocus="true"
>+    <panel id="identity-popup" type="arrow" position="after_start" hidden="true" noautofocus="true"
>            onpopupshown="document.getElementById('identity-popup-more-info-button').focus();"
>-           level="top">
>+           level="parent">

Same here.

>      tooltip appearance for the address bar popup panels */
>   #identity-popup,
>   #editBookmarkPanel {
>-    -moz-appearance: tooltip;
>+    -moz-appearance: none;
>     color: InfoText;

Not sure what this change is about.

Comment 6

[Marco Bonardo [:mak]]

please check functionality does not regress, for example the panel enlarges when the folder picker is opened, does that still work correctly? Also I suggest a run of browser-chrome tests in browser/components/places, just in case.

Comment 7

[:Margaret Leibovic]

(In reply to comment #4)
> (In reply to comment #3)
> Ok. I don't think that conflicts with this bug or the patch provided though.

The styles won't conflict, but your patch will make them unused, so it would be a good idea to remove them :)

> Btw, I noticed that the arrow panels don't align perfectly with the anchorNode
> now, since panels align based on the position attribute (or argument passed to
> openPopup). That doesn't give you the option of aligning with the arrow. Is
> there a bug for that or is it covered somewhere?

I'm working on that in my patch for bug 577928.

Comment 8

[Nochum Sossonko [:Natch]]

(In reply to comment #5)
> >+           level="parent"
> 
> Is thee are reason you need to set the level here? This will likely break IME
> usage.

Remnants of a previous attempt, will get rid of it as the change is unnecessary.

> >-    -moz-appearance: tooltip;
> >+    -moz-appearance: none;
> >     color: InfoText;
> 
> Not sure what this change is about.

-moz-appearance: tooltip makes the panel encased in a white tooltip bubble.

New patch coming.

Comment 9

[Nochum Sossonko [:Natch]]

Attachment: patch

Addresses comments. Also, I ran the tests in browser/components/places and all went fine. I'm gonna push to try to check other platforms as well.

The panel still expands just as it did before, no problems.

Comment 10

[Neil Deakin]

> -moz-appearance: tooltip makes the panel encased in a white tooltip bubble.

But why are you setting it to 'none' instead? The line should just be left out.

Comment 11

[Nochum Sossonko [:Natch]]

(In reply to comment #10)
> > -moz-appearance: tooltip makes the panel encased in a white tooltip bubble.
> 
> But why are you setting it to 'none' instead? The line should just be left out.

Because browser.css from winstripe sets it already. browser-aero.css was overriding that. Let me give it a spin with it just left out...

OTOH, should I remove the -moz-appearance from browser.css as well (for non-aero windows)?

Comment 12

[Nochum Sossonko [:Natch]]

Attachment: patch

Adjusted to remove the -moz-appearance stuff altogether.

Comment 13

[Nochum Sossonko [:Natch]]

Btw, I tagged you for feedback since I wasn't sure if you can review the browser/ stuff, if you can, please do...and I'll just take your r+ and land (not having to bother gavin).

Thank you for your time :)

Comment 14

[Nochum Sossonko [:Natch]]

Try seems to like this, modulo some known [orange] errors (there was debug and opt coverage on the run and whatever failed in one passed in the other anyhow). This is good to go :)

Comment 15

[Justin Dolske [:Dolske]]

Logistics note: I think we shouldn't take this until bug 606343 makes arrow panels point at the right location.

Comment 16

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

Neil: does switching to an arrowpanel allow us to make this user resizeable?

Comment 17

[Marco Bonardo [:mak]]

The problem with bookmarks dialog is not making it resizeable, but making it resizeable on the bottom-left side with a min-width. Till a couple months ago trying to do that was completely messing up the dialog (something like creating a ghost border without content) and moving content on the right while resizing it.
That is probably related to wrong panel coords calculation with dialog attachment, not sure if recent improvements have made that better. Making the dialog resizeable on the bottom-right should be trivial instead (even if most likely min-width is still unsupported and users could make the dialog disappear).

Comment 19

[Johnathan Nightingale [:johnath]]

Morphing this to reflect what the patch actually does, and duping bug 597556 in.

Comment 20

[Nochum Sossonko [:Natch]]

(In reply to comment #17)
> The problem with bookmarks dialog is not making it resizeable, but making it
> resizeable on the bottom-left side with a min-width. Till a couple months ago
> trying to do that was completely messing up the dialog (something like creating
> a ghost border without content) and moving content on the right while resizing
> it.

This could very well be related to the appearance styling it was getting (if it was) since menupopup and tooltip -moz-appearance cause it to get "bubbled" under certain circumstances. This patch has to remove that appearance styling anyhow, so it might work after this patch...

Comment 21

[Neil Deakin]

Comment on attachment 483509 [details] [diff] [review]
patch

This patch only implements Windows, and, in fact, breaks Mac.

Comment 22

[Nochum Sossonko [:Natch]]

(In reply to comment #21)
> Comment on attachment 483509 [details] [diff] [review]
> patch
> 
> This patch only implements Windows, and, in fact, breaks Mac.

Can you attach a screenshot? I don't have a mac unfortunately...

Comment 23

[:Margaret Leibovic]

Attachment: patch with mac style changes

I added some mac style changes to the patch to make it work on osx. However, we probably need to make similar changes to the other themes. Basically, we need to get rid of any styles that will override the arrow panel styling.

Also, I don't know if we want to address the styling of the inside of the panels in this bug, but the button styles eventually need to be updated to match the style of the updated popup notification button styles.

Natch, if you want I can take over this bug, since I'm working on the popup notification styling bugs, or you can ask me questions if you need help :)

Comment 24

[Nochum Sossonko [:Natch]]

Ye, take it over. I don't have the resources to drive this through on all platforms. :(

Thanks!

Comment 25

[:Margaret Leibovic]

Attachment: WIP

I'm working on updating the button styling in this patch, since we're already editing the panel styles to convert them to arrow panels.

Dão, I tried putting the button styles in toolkit, but I was having trouble getting the buttons to pick them up. I assume it's because the buttons are being made in browser.xul, not the xbl. As a work around I decided to just declare styles for a .panel-arrow-button class in browser.css. Do you think this is a good approach, or is there a better solution?

Comment 26

[Dão Gottwald [:dao]]

(In reply to comment #25)
> Created attachment 488140 [details] [diff] [review]
> WIP
> 
> I'm working on updating the button styling in this patch, since we're already
> editing the panel styles to convert them to arrow panels.

Please avoid, it makes the whole undertaking unnecessarily complicated. Note that editBookmarkOverlay.xul is used in different contexts. Feel free to update @hudButton@, though -- that should cover the relevant cases on Mac -- and file a follow-up bug for Windows.

Comment 27

[:Margaret Leibovic]

Attachment: patch

I updated the pinstripe theme in this patch. I updated the hud button styles, and I updated the rest of the edit bookmark panel to include work that Stephen had done in the past. If this is out of scope for this bug, let me know and I'll file a new bug.

I filed bug 610566 for the follow-up winstripe theme work.

Comment 28

[Dão Gottwald [:dao]]

Comment on attachment 489072 [details] [diff] [review]
patch

Please remove unused images.

>+  border:  1px solid rgba(0,0,0,.3);

nit: double space (you have this a couple of times)

>+  color: #ffffff;

nit: #fff or white

Comment 29

[Dão Gottwald [:dao]]

Comment on attachment 489072 [details] [diff] [review]
patch

>--- a/browser/themes/pinstripe/browser/shared.inc
>+++ b/browser/themes/pinstripe/browser/shared.inc

>\ No newline at end of file

Please fix this as well.

Comment 30

[:Margaret Leibovic]

Attachment: patch

Comment 31

[Dão Gottwald [:dao]]

Comment on attachment 489195 [details] [diff] [review]
patch

> #editBMPanel_tagsField,
> #editBMPanel_namePicker[droppable="false"] > .menulist-editable-box {
>   -moz-appearance: none !important;
>-  margin: 2px 4px !important;
>-  border: 2px solid !important;
>-  -moz-border-top-colors: #1c1c1c #545454 !important;
>-  -moz-border-right-colors: #1c1c1c #636363 !important;
>-  -moz-border-bottom-colors: #1c1c1c #797979 !important;
>-  -moz-border-left-colors: #1c1c1c #636363 !important;
>-  border-radius: 1px !important;
>+  margin: 2px !important;
>+  border: 1px solid rgba(0,0,0,.5) !important;
>+  box-shadow: inset 0 1px 0 rgba(0,0,0,.3);
>   background-color: #666 !important;
>+  background-clip: padding-box;
>+  background-origin: padding-box;
>   color: #fff !important;
>+  min-height: 20px;
> }
> 
>+#editBMPanel_tagsField,
> #editBMPanel_namePicker[droppable="false"] > .menulist-editable-box > html|*.menulist-editable-input {
>   color: inherit;
>+  -moz-padding-start: 3px !important;
> }

Can you explain why you're adding the padding at different levels for the two fields?
(color: inherit; also doesn't make sense for #editBMPanel_tagsField.)

Comment 32

[:Margaret Leibovic]

(In reply to comment #31)
> Can you explain why you're adding the padding at different levels for the two
> fields?
> (color: inherit; also doesn't make sense for #editBMPanel_tagsField.)

#editBMPanel_tagsField is a textbox and #editBMPanel_namePicker is a menulist, so for the same padding effect, I had to add the padding at different levels.

I can make a new style rule to avoid applying the color: inherit; to #editBMPanel_tagsField.

Comment 33

[Dão Gottwald [:dao]]

(In reply to comment #32)
> (In reply to comment #31)
> > Can you explain why you're adding the padding at different levels for the two
> > fields?
> > (color: inherit; also doesn't make sense for #editBMPanel_tagsField.)
> 
> #editBMPanel_tagsField is a textbox and #editBMPanel_namePicker is a menulist,
> so for the same padding effect, I had to add the padding at different levels.

This doesn't explain why you chose html|*.menulist-editable-input rather than .menulist-editable-box (which is used in the rule above).

Comment 34

[:Margaret Leibovic]

Attachment: patch v2

Looking at this more closely, that padding rule should be in the first style block. I think it just made it's way into that second block when I was experimenting with the styles.

Comment 35

[Dão Gottwald [:dao]]

Comment on attachment 489311 [details] [diff] [review]
patch v2

> #editBMPanel_tagsField,
> #editBMPanel_namePicker[droppable="false"] > .menulist-editable-box {
>   -moz-appearance: none !important;
>-  margin: 2px 4px !important;
>-  border: 2px solid !important;
>-  -moz-border-top-colors: #1c1c1c #545454 !important;
>-  -moz-border-right-colors: #1c1c1c #636363 !important;
>-  -moz-border-bottom-colors: #1c1c1c #797979 !important;
>-  -moz-border-left-colors: #1c1c1c #636363 !important;
>-  border-radius: 1px !important;
>+  -moz-padding-start: 3px !important;
>+  margin: 2px !important;
>+  border: 1px solid rgba(0,0,0,.5) !important;
>+  box-shadow: inset 0 1px 0 rgba(0,0,0,.3);
>   background-color: #666 !important;
>+  background-clip: padding-box;
>+  background-origin: padding-box;
>   color: #fff !important;
>+  min-height: 20px;
> }

I don't think any of these !important flags should be needed. Can you please double-check?

:-moz-focusring should probably be used instead of :focus throughout this code.

Comment 36

[:Margaret Leibovic]

Attachment: patch v3

The !important flags were needed for #editBMPanel_namePicker to pick up the styles. I assume it has to do with the fact that it's an editable menulist. I tried playing around with some of the toolkit styles to see if I could find what was causing this, but there wasn't any obvious fix.

Comment 37

[imradyurrad]

What's left but implementation?

Comment 38

[:Margaret Leibovic]

(In reply to comment #37)
> What's left but implementation?

The patch is done, but we're waiting for bug 606343 to prevent the appearance of a regression on OSX.

Comment 40

[Neil Deakin]

http://hg.mozilla.org/mozilla-central/rev/62d5d8bc0ac8