Closed
Bug 471838
Opened 16 years ago
Closed 15 years ago
Help->About "Licensing information" text is wrapped after the opening parenthesis
Categories
(Firefox :: General, defect)
Firefox
General
Tracking
()
RESOLVED
INVALID
People
(Reporter: Gavin, Unassigned)
References
(Depends on 1 open bug)
Details
(Keywords: polish, Whiteboard: [polish-easy] [polish-visual][polish-p2])
Attachments
(3 files, 2 obsolete files)
(deleted),
image/png
|
Details | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
image/png
|
Details |
Wrapping after the opening parenthesis is unfortunate. It'd be nicer if the entire line would be kept together on it's own line. We can probably force that by adding an extra space or two before it.
Reporter | ||
Comment 1•16 years ago
|
||
Updated•16 years ago
|
Whiteboard: [polish-easy] [polish-visual]
Comment 3•16 years ago
|
||
Do we really need parentheses here? I'd say we should remove them.
Reporter | ||
Comment 4•16 years ago
|
||
That sounds reasonable, but we're past the l10n freeze. We might be able to get away with an automated change to all locales, though, since this isn't something that varies much, I think.
Comment 5•16 years ago
|
||
Why do we line break immediately after a parenthesis anyway? Is there a bogus space in the source that there shouldn't be?
Gerv
Comment 6•16 years ago
|
||
<checks> No, no bogus space. It's a bug in our text-wrapping algorithm (IMO, anyway).
It may well only occur in English, and maybe only on Mac OS, because you'd need text of the right length (and therefore in a certain font). We could remove the parentheses in English only. :-)
Gerv
Comment 7•16 years ago
|
||
Occurs on Linux, too.
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2a1pre) Gecko/20090114 Minefield/3.2a1pre ID:20090114020416
Reporter | ||
Comment 8•16 years ago
|
||
bz points out that this happens because the inner text is in a separate <label> (which is a child of the <description> that holds the wrapping lines with parentheses), and that that makes this a duplicate of a WONTFIX bug (which I can't find). We should just work around it.
Comment 9•16 years ago
|
||
The bug I was thinking of is bug 472126, of which this looks like a duplicate.
Reporter | ||
Comment 10•16 years ago
|
||
Well, we can fix this one without fixing that one (by using a workaround in the dialog XUL), so not quite duplicates I would say.
Depends on: 472126
Comment 12•16 years ago
|
||
Just posting it here before I drop it, this was the only way I could get the "(" to show up on the next line (aside from making l10n changes).
Updated•16 years ago
|
Assignee: nobody → dao
Comment 13•16 years ago
|
||
Attachment #368816 -
Flags: review?(gavin.sharp)
Reporter | ||
Comment 14•16 years ago
|
||
Comment on attachment 368816 [details] [diff] [review]
patch
>diff --git a/browser/base/content/aboutDialog.css b/browser/base/content/aboutDialog.css
>+html|a:focus {
>+ outline: 1px dotted;
Why outline instead of border? Should you specify -moz-DialogText as the toolkit themes do for their border?
What about the color changes (red for focus)?
This seems to break keyboard access (can't tab to the link and press enter). Strangely enough, it seems to work if I use Ctrl+Enter (!?).
Comment 15•16 years ago
|
||
(In reply to comment #14)
> Why outline instead of border? Should you specify -moz-DialogText as the
> toolkit themes do for their border?
> What about the color changes (red for focus)?
Outline is the standard tool for drawing focus rings.
The text-link styling in global.css seems arbitrary... Red for :focus doesn't make much sense. In HTML it's usually just red for :active [1][2], but even that wouldn't be right for native apps on Windows.
I could copy the styling from global.css, but it doesn't make much sense to me.
[1]: http://hg.mozilla.org/mozilla-central/annotate/526a987a3b1e/layout/style/ua.css#l114
[2]: http://hg.mozilla.org/mozilla-central/annotate/526a987a3b1e/layout/base/nsPresShell.cpp#l2158
Comment 16•16 years ago
|
||
Comment on attachment 368816 [details] [diff] [review]
patch
Need to fix the keyboard access
Attachment #368816 -
Attachment is obsolete: true
Attachment #368816 -
Flags: review?(gavin.sharp)
Reporter | ||
Comment 17•16 years ago
|
||
Ok, do we have bugs on file on fixing the toolkit .text-link styling?
Comment 18•16 years ago
|
||
Attachment #369134 -
Flags: review?(gavin.sharp)
Comment 19•16 years ago
|
||
(In reply to comment #17)
> Ok, do we have bugs on file on fixing the toolkit .text-link styling?
I don't think so, I'll file one.
Reporter | ||
Comment 20•16 years ago
|
||
Comment on attachment 369134 [details] [diff] [review]
patch v2
>diff --git a/toolkit/content/widgets/text.xml b/toolkit/content/widgets/text.xml
>- <handler event="keypress" preventdefault="true" keycode="VK_ENTER" action="this.click()" />
>- <handler event="keypress" preventdefault="true" keycode="VK_RETURN" action="this.click()" />
>+ <handler event="keypress" keycode="VK_ENTER" action="this._handleEnter(event);"/>
>+ <handler event="keypress" keycode="VK_RETURN" action="this._handleEnter(event);"/>
This preventdefault removal seems to be sufficient to fix keyboard access... why the other changes?
Comment 21•16 years ago
|
||
This gives you "Error: this.click is not a function" in the error console, and we'd bypass the security check. I'm also not sure that not doing preventDefault for the keypress event would be a good idea for text-link labels.
Reporter | ||
Comment 22•16 years ago
|
||
Oh... why don't we just call open() directly in both cases? Not sure I see what calling click() gets us.
What default action are we preventing in the label case? I don't see any negative side effects in the engine manager with preventdefault="true" removed (no system beep, etc.).
Comment 23•16 years ago
|
||
(In reply to comment #22)
> Oh... why don't we just call open() directly in both cases? Not sure I see what
> calling click() gets us.
That would be ideal, but it would also break the engine manager (and others), as it uses onclick but no href attribute.
> What default action are we preventing in the label case? I don't see any
> negative side effects in the engine manager with preventdefault="true" removed
> (no system beep, etc.).
The label could be inside of some thing that consumes the event depending on getPreventDefault().
Comment 24•16 years ago
|
||
(In reply to comment #23)
> (In reply to comment #22)
> > Oh... why don't we just call open() directly in both cases? Not sure I see what
> > calling click() gets us.
>
> That would be ideal, but it would also break the engine manager (and others),
> as it uses onclick but no href attribute.
likely because of bug 263433
Updated•15 years ago
|
Flags: wanted-firefox3.6?
Comment 25•15 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.
Unlike the focus ring problem, this is more clearly broken.
Whiteboard: [polish-easy] [polish-visual] → [polish-easy] [polish-visual][polish-p2]
Comment 27•15 years ago
|
||
Confirmed in FF 3.5 final en_US.
Comment 28•15 years ago
|
||
Updated•15 years ago
|
Flags: wanted-firefox3.5?
Updated•15 years ago
|
Attachment #364734 -
Attachment is obsolete: true
Updated•15 years ago
|
Flags: wanted-firefox3.6?
Comment 29•15 years ago
|
||
This bug is no longer valid now that the patch in bug 536336 has landed.
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → INVALID
Updated•15 years ago
|
Assignee: dao → nobody
Updated•15 years ago
|
Attachment #369134 -
Flags: review?(gavin.sharp)
You need to log in
before you can comment on or make changes to this bug.
Description
•