Closed
Bug 484022
Opened 16 years ago
Closed 16 years ago
Title and button in the "Edit Bookmark" panel should be aligned with the fields, star should be centered
Categories
(Firefox :: Bookmarks & History, defect)
Tracking
()
VERIFIED
FIXED
Firefox 3.6a1
People
(Reporter: frandavid100, Assigned: dao)
References
Details
(Keywords: verified1.9.1, Whiteboard: [polish-easy][polish-visual][polish-p2])
Attachments
(4 files, 5 obsolete files)
(deleted),
image/png
|
Details | |
(deleted),
image/png
|
Details | |
(deleted),
patch
|
beltzner
:
approval1.9.1+
|
Details | Diff | Splinter Review |
(deleted),
image/jpeg
|
Details |
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
Reporter | ||
Comment 1•16 years ago
|
||
Comment 2•16 years ago
|
||
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.
Version: unspecified → 3.0 Branch
Reporter | ||
Comment 3•16 years ago
|
||
Well, I can say it does look like the screenshot in linux.
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
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: wanted-firefox3.5?
Whiteboard: [polish-needed]
Version: 3.0 Branch → 3.1 Branch
Comment 5•16 years ago
|
||
CC'ing ddahl since he might be interested. :)
Comment 6•16 years ago
|
||
yeah, we should fix this. In addition to looking better, this also make the layout consistent with other platforms like OS X.
Whiteboard: [polish-needed] → [polish-easy][polish-visual]
Comment 7•16 years ago
|
||
In addition to lining up the title and remove button, we should center the star.
Comment 8•16 years ago
|
||
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.
Assignee | ||
Updated•16 years ago
|
Assignee: nobody → dao
OS: Linux → All
Hardware: x86 → All
Assignee | ||
Updated•16 years ago
|
Summary: The bookmark editor would certainly look better if all the buttons were aligned to the left. → Title and button in the "Edit Bookmark" panel should be aligned with the fields, star should be centered
Assignee | ||
Comment 9•16 years ago
|
||
Attachment #369110 -
Flags: review?(mak77)
Assignee | ||
Comment 10•16 years ago
|
||
fixed a typo
Attachment #369110 -
Attachment is obsolete: true
Attachment #369112 -
Flags: review?(mak77)
Attachment #369110 -
Flags: review?(mak77)
Assignee | ||
Comment 11•16 years ago
|
||
note that I haven't tested this with pinstripe yet...
Assignee | ||
Comment 12•16 years ago
|
||
This might fix an issue with pinstripe...
Btw, I think we could use browser.dtd in editBookmarkOverlay.xul to fix this on branch.
Attachment #369112 -
Attachment is obsolete: true
Attachment #369114 -
Flags: review?(mak77)
Attachment #369112 -
Flags: review?(mak77)
Comment 13•16 years ago
|
||
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
Assignee | ||
Comment 14•16 years ago
|
||
(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.
Assignee | ||
Comment 15•16 years ago
|
||
(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.
Assignee | ||
Comment 16•16 years ago
|
||
Comment on attachment 369114 [details] [diff] [review]
patch v2
Trying something different, not sure if it will work.
Attachment #369114 -
Flags: review?(mak77)
Comment 17•16 years ago
|
||
(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.
Assignee | ||
Comment 18•16 years ago
|
||
Attachment #369114 -
Attachment is obsolete: true
Attachment #369261 -
Flags: review?(mak77)
Updated•16 years ago
|
Attachment #369261 -
Flags: review?(mak77) → review+
Comment 19•16 years ago
|
||
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
Assignee | ||
Comment 20•16 years ago
|
||
(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.
Assignee | ||
Comment 21•16 years ago
|
||
(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•16 years ago
|
||
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.
Assignee | ||
Comment 23•16 years ago
|
||
added a comment and margin between the content and the bottom buttons
Attachment #369261 -
Attachment is obsolete: true
Assignee | ||
Comment 24•16 years ago
|
||
(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.
Assignee | ||
Comment 25•16 years ago
|
||
In fact, we couldn't load that overlay twice in the browser scope, as it would give us duplicate ids.
Comment 26•16 years ago
|
||
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.
Assignee | ||
Comment 27•16 years ago
|
||
added another comment and fixed the tags selector
Attachment #369280 -
Attachment is obsolete: true
Assignee | ||
Comment 28•16 years ago
|
||
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.6a1
Assignee | ||
Updated•16 years ago
|
Attachment #369284 -
Flags: approval1.9.1?
Comment 29•16 years ago
|
||
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•16 years ago
|
||
Assignee | ||
Comment 31•16 years ago
|
||
It's possible that there's a margin floating around that we don't want. File a new bug, please?
Comment 32•16 years ago
|
||
Filed bug 485499
Comment 33•16 years ago
|
||
Comment on attachment 369284 [details] [diff] [review]
patch v3.2
a191=beltzner
Attachment #369284 -
Flags: approval1.9.1? → approval1.9.1+
Assignee | ||
Updated•16 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 34•16 years ago
|
||
Keywords: checkin-needed → fixed1.9.1
Comment 35•16 years ago
|
||
This patch has been regressed bug 488255 on OS X.
Assignee: dao → nobody
Component: Bookmarks & History → Places
Depends on: 488255
QA Contact: bookmarks → places
Updated•16 years ago
|
Assignee: nobody → dao
Assignee | ||
Updated•16 years ago
|
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
Comment 36•16 years ago
|
||
verified with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b4pre)
Gecko/20090414 Shiretoko/3.5b4pre
Status: RESOLVED → VERIFIED
Keywords: fixed1.9.1 → verified1.9.1
Updated•16 years ago
|
Flags: wanted-firefox3.5?
Comment 37•16 years ago
|
||
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.
Whiteboard: [polish-easy][polish-visual] → [polish-easy][polish-visual][polish-p2]
See Also: → https://launchpad.net/bugs/240614
You need to log in
before you can comment on or make changes to this bug.
Description
•