484022 - Title and button in the "Edit Bookmark" panel should be aligned with the fields, star should be centered

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[David Prieto]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.0.7) Gecko/2009030516 Ubuntu/9.04 (jaunty) Firefox/3.0.7
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.0.7) Gecko/2009030516 Ubuntu/9.04 (jaunty) Firefox/3.0.7

I'm attaching a mockup.

Reproducible: Always

Comment 1

[David Prieto]

Attachment: screenshot and mockup

Comment 2

[Tyler Downer [He/Him]]

That works for me in Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.0.7) Gecko/2009021910 Firefox/3.0.7 (.NET CLR 3.5.30729) ID:2009021910. Maybe different in linux.

Comment 3

[David Prieto]

Well, I can say it does look like the screenshot in linux.

Comment 4

[cmtalbert]

I can confirm this on Shiretoko (beta 3): Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1b3) Gecko/20090305 Firefox/3.1b3

And also on: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2a1pre) Gecko/20090319 Minefield/3.6a1pre

It looks like the "Remove Bookmark" button is about 5px to the left and so it doesn't align with the text boxes below it.  The effect is even more extreme on the localized build that David attached a screen shot for.  This little dialog is extremely different on each OS, so this is a linux only issue.  It seems like a simple polish fix, adding polish-needed whiteboard and ccing Mr. Faaborg to decide on how to further triage.

--> Confirmed

Comment 5

[Drew Willcoxon :adw]

CC'ing ddahl since he might be interested. :)

Comment 6

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

yeah, we should fix this.  In addition to looking better, this also make the layout consistent with other platforms like OS X.

Comment 7

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

Attachment: Minor tweak: center the star

In addition to lining up the title and remove button, we should center the star.

Comment 8

[Marco Bonardo [:mak]]

i have no idea of how we could do that, the internal elements (label|field) are managed through a grid, and the left part of the grid enlarges based on the contained text (that can also change for different locales).
I would say instead we should take old dao's suggestion to make the upper part of the dialog have a different style than the bottom part. Unless someone has an idea of how to align with the size of a grid column we are out of.

Comment 9

[Dão Gottwald [:dao]]

Attachment: patch

Comment 10

[Dão Gottwald [:dao]]

Attachment: patch

fixed a typo

Comment 11

[Dão Gottwald [:dao]]

note that I haven't tested this with pinstripe yet...

Comment 12

[Dão Gottwald [:dao]]

Attachment: patch v2

This might fix an issue with pinstripe...

Btw, I think we could use browser.dtd in editBookmarkOverlay.xul to fix this on branch.

Comment 13

[Marco Bonardo [:mak]]

well, this is what i did not want to do, add code specific to a certain panel to the shared overlay used in all bookmarks dialogs, ideally all code relative to StarUI object should not go into the overlay.
would not be possible to get dynamically the width of the first column of the grid and apply it to a box containing the star icon?
If we could find an alternative to polluting editItemOverlay i would largely prefer that, since this adds useless code to all bookmarks dialogs and those buttons refer to a non existant (there) starUI object that's confusing.
Also accesskeys could overlap, for example E is used for more/less in Library.
So, i'm not sure the cost/benefit of this change is so good

Comment 14

[Dão Gottwald [:dao]]

(In reply to comment #13)
> well, this is what i did not want to do, add code specific to a certain panel
> to the shared overlay used in all bookmarks dialogs

Seems to be already the case the other way around with editBMPanel_itemsCountText.

> would not be possible to get dynamically the width of the first column of the
> grid and apply it to a box containing the star icon?

That seems to me like an far worse hack...

> Also accesskeys could overlap, for example E is used for more/less in Library.

I don't think that will be an issue, since the header is hidden by default, but I haven't tested that case yet.

Comment 15

[Dão Gottwald [:dao]]

(In reply to comment #14)
> > would not be possible to get dynamically the width of the first column of the
> > grid and apply it to a box containing the star icon?
> 
> That seems to me like an far worse hack...

Assuming "dynamically" means "with a script", because XUL doesn't provide this.

Comment 16

[Dão Gottwald [:dao]]

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

Trying something different, not sure if it will work.

Comment 17

[Marco Bonardo [:mak]]

(In reply to comment #14)
> (In reply to comment #13)
> Seems to be already the case the other way around with
> editBMPanel_itemsCountText.

not completely, multiple item edit is a supported feature of the panel, and even if that is actually only used in the Library could be used in other places (for example when doing Bookmark all tabs), the editItemOverlay object knows how many items it's editing. This is not to be confused with "itemsCountBox" that shows number of selected elements in the Library.

> 
> > would not be possible to get dynamically the width of the first column of the
> > grid and apply it to a box containing the star icon?
> 
> That seems to me like an far worse hack...

it is, but would be self contained in that panel and in StarUI object, with dynamically i meant through starUI code clearly.

Comment 18

[Dão Gottwald [:dao]]

Attachment: patch v3

Comment 19

[Marco Bonardo [:mak]]

Comment on attachment 369261 [details] [diff] [review]
patch v3

mh, looks like we still have the issue with the panel enlarging by a couple px when Tags selector is open :\
In this case is probably due to the fact the star takes up more space than our longest label, but would also happen for longer labels.
I would say to file a followup on that (or reopen the old bug that i can't find atm), unless you have some idea to avoid the issue (or we could enlarge the panel, i see it on xp.

>diff --git a/browser/base/content/browser-places.js b/browser/base/content/browser-places.js
>--- a/browser/base/content/browser-places.js
>+++ b/browser/base/content/browser-places.js
>@@ -168,16 +168,21 @@ var StarUI = {
>   },
> 
>   _doShowEditBookmarkPanel:
>   function SU__doShowEditBookmarkPanel(aItemId, aAnchorElement, aPosition) {
>     if (this.panel.state != "closed")
>       return;
> 
>     this._blockCommands(); // un-done in the popuphiding handler
>+
>+    var rows = this._element("editBookmarkPanelGrid").lastChild;
>+    var header = this._element("editBookmarkPanelHeader");
>+    rows.insertBefore(header, rows.firstChild);
>+    header.hidden = false;

Please add a comment above this code with a brief explanation of why we do this.

>diff --git a/browser/themes/pinstripe/browser/browser.css b/browser/themes/pinstripe/browser/browser.css

>-#editBookmarkPanel #editBMPanel_newFolderBox {
>+#editBMPanel_newFolderBox {

even if this is correct since this is in browser.css and won't be applied to other instances, i fear the change could be error-prone, someone could think to move some of these styles to the shared editItemOverlay.css. I think was done with this purpose, moreover if we would add a new panel instance in the sidebar (i don't think we will but, who knows) styles would be applied there.
Personally i'd prefer to retain the useless #editBookmarkPanel selectors in front of shared elements if the style should apply only to the star panel, but is clearly a personal vision.

>diff --git a/browser/themes/winstripe/browser/browser.css b/browser/themes/winstripe/browser/browser.css
>--- a/browser/themes/winstripe/browser/browser.css
>+++ b/browser/themes/winstripe/browser/browser.css
>@@ -1258,16 +1258,20 @@ statusbarpanel#statusbar-display {
> #editBookmarkPanelStarIcon[unstarred] {
>   list-style-image: url("chrome://browser/skin/places/unstarred48.png");
> }
> 
> #editBookmarkPanelTitle {
>   font-size: 130%;
> }
> 
>+#editBookmarkPanelHeader {
>+  margin-bottom: .5em;
>+}
>+

could you please add some top marging before the bottom buttons too, they're simply too much attached to above fields

otherwise looks good

Comment 20

[Dão Gottwald [:dao]]

(In reply to comment #19)
> even if this is correct since this is in browser.css and won't be applied to
> other instances, i fear the change could be error-prone, someone could think to
> move some of these styles to the shared editItemOverlay.css. I think was done
> with this purpose, moreover if we would add a new panel instance in the sidebar
> (i don't think we will but, who knows) styles would be applied there.
> Personally i'd prefer to retain the useless #editBookmarkPanel selectors in
> front of shared elements if the style should apply only to the star panel, but
> is clearly a personal vision.

It violates <https://developer.mozilla.org/en/Writing_Efficient_CSS>. To address your concern, I'd rather add /*editBookmarkPanel*/ in front of each rule, or move that stuff to browser_editBookmarkPanel.css.inc, or something like that.

Comment 21

[Dão Gottwald [:dao]]

(In reply to comment #19)
> mh, looks like we still have the issue with the panel enlarging by a couple px
> when Tags selector is open :\
> In this case is probably due to the fact the star takes up more space than our
> longest label, but would also happen for longer labels.
> I would say to file a followup on that (or reopen the old bug that i can't find
> atm), unless you have some idea to avoid the issue (or we could enlarge the
> panel, i see it on xp.

I can't reproduce this on Linux. I'll try it on XP.

Comment 22

[Marco Bonardo [:mak]]

I know it's not as efficient as it could be, but it's not a violation if those styles should be applied only to one specific instance of a shared overlay, i mean, those are not there only because they're pretty :)
We can also omit those, they are quite useless atm, but if in future we will need to add another instance of the overlay in browser, those will have to be added back. All i can say is that it shouldn't happen soon, afaik.

Comment 23

[Dão Gottwald [:dao]]

Attachment: patch v3.1

added a comment and margin between the content and the bottom buttons

Comment 24

[Dão Gottwald [:dao]]

(In reply to comment #22)
> if in future we will
> need to add another instance of the overlay in browser, those will have to be
> added back.

That wouldn't apply to the sidebar, btw, as the sidebar content is a separate document.

Comment 25

[Dão Gottwald [:dao]]

In fact, we couldn't load that overlay twice in the browser scope, as it would give us duplicate ids.

Comment 26

[Marco Bonardo [:mak]]

ok, i'm sold!
if possible please add a single comment in browser.css before editBMPanel styles specifying those are specific for the star panel and should not be moved to the shared overlay stylesheet.

Comment 27

[Dão Gottwald [:dao]]

Attachment: patch v3.2

added another comment and fixed the tags selector

Comment 28

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/923816ab8dab

Comment 29

[Luke Iliffe (Harlequin99)]

Is it just me or is the "Remove bookmark" button and the "Edit this bookmark" title still 1px to the right of the left border of the text fields below. Screenshot coming from XP with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090326 Minefield/3.6a1pre ID:20090326050203

Comment 30

[Luke Iliffe (Harlequin99)]

Attachment: Title and button still offset to the right

Comment 31

[Dão Gottwald [:dao]]

It's possible that there's a margin floating around that we don't want. File a new bug, please?

Comment 32

[Luke Iliffe (Harlequin99)]

Filed bug 485499

Comment 33

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 369284 [details] [diff] [review]
patch v3.2

a191=beltzner

Comment 34

[Dão Gottwald [:dao]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/33b15e439e95

Comment 35

[Henrik Skupin [:whimboo][⌚️UTC+1]]

This patch has been regressed bug 488255 on OS X.

Comment 36

[Tracy Walker [:tracy]]

verified with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b4pre)
Gecko/20090414 Shiretoko/3.5b4pre

Comment 37

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

This bug's priority relative to the set of other polish bugs is:
P2 - Polish issue that is in a secondary interface, occasionally encountered, and is easily identifiable.