align the buttons in the bookmark editor

[David Prieto]

Binary package hint: firefox-3.0

The bookmark editor would certainly look better if all the buttons were aligned to the left. I'm attaching a mockup.

[Pedro Villavicencio (pedro)]

something to be send to bugzilla.mozilla.org, that's not an ubuntu task. opening an upstream task so we don't forget about it.

[Pedro Villavicencio (pedro)]

ping David, did you sent this upstream? may you tell us the bug number there or shall we go ahead and close this report?

[John Vivirito (gnomefreak)]

Please change this back to new and place link the URL line once you file it or find it upstream. I do rmember seeing this eithe rin LP or upstream but it was a while ago.

[David Prieto (frandavid100-gmail)]

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

[David Prieto (frandavid100-gmail)]

Created attachment 368049
screenshot and mockup

[David Prieto (frandavid100-gmail)]

I just sent the bug upstream: https://bugzilla.mozilla.org/show_bug.cgi?id=484022

[Tyler Downer (tyler-downer)]

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.

[David Prieto (frandavid100-gmail)]

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

[ctalbert (ctalbert)]

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

[Adw-mozilla (adw-mozilla)]

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

[Faaborg (faaborg)]

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

[Faaborg (faaborg)]

Created attachment 368399
Minor tweak: center the star

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

[Mak77 (mak77)]

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.

[Dao (dao)]

Created attachment 369110
patch

[Dao (dao)]

Created attachment 369112
patch

fixed a typo

[Dao (dao)]

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

[Dao (dao)]

Created attachment 369114
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.

[Mak77 (mak77)]

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

[Dao (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.

[Dao (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.

[Dao (dao)]

Comment on attachment 369114
patch v2

Trying something different, not sure if it will work.

[Mak77 (mak77)]

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

[Dao (dao)]

Created attachment 369261
patch v3

[Mak77 (mak77)]

Comment on attachment 369261
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

[Dao (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.

[Dao (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.

[Mak77 (mak77)]

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.

[Dao (dao)]

Created attachment 369280
patch v3.1

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

[Dao (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.

[Dao (dao)]

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

[Mak77 (mak77)]

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.

[Dao (dao)]

Created attachment 369284
patch v3.2

added another comment and fixed the tags selector

[Dao (dao)]

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

[Luke-iliffe (luke-iliffe)]

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

[Luke-iliffe (luke-iliffe)]

Created attachment 369506
Title and button still offset to the right

[Dao (dao)]

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

[Luke-iliffe (luke-iliffe)]

Filed bug 485499

[Beltzner (beltzner)]

Comment on attachment 369284
patch v3.2

a191=beltzner

[Dao (dao)]

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

[Hskupin (hskupin)]

This patch has been regressed bug 488255 on OS X.

[Twalker (twalker)]

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

[Faaborg (faaborg)]

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.

[Micah Gersten (micahg)]

I believe this was released with the initial release of Firefox 3.5.

[Micah Gersten (micahg)]

Firefox 3.0 is only receiving Security Updates and major bug fixes at this point.

[David Prieto (frandavid100-gmail)]

Micah, it was released with FF 3.5 indeed.

Thanks!