Matches containing ligatures in awesomebar results should not be underlined, use normal style instead

RESOLVED FIXED in Firefox 16

Status

()

Firefox
Location Bar
--
minor
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: ahurle, Assigned: ahurle)

Tracking

({regression})

Trunk
Firefox 17
regression
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(firefox16 fixed)

Details

Attachments

(2 attachments)

(Assignee)

Description

5 years ago
Created attachment 642763 [details]
Screenshot showing current emphasis on the left, desired change on the right

In bug 407861, an underline was introduced for matches which had ligatures since bolding the text would break ligatures.  Now that we're using a highlighting style without bold text, all matches should use that new style since it shouldn't break anything.

Should be able to just get rid of the logic that checks for alt emphasis, and use ac-emphasize-text on all matches instead.
(Assignee)

Comment 1

5 years ago
Created attachment 642853 [details] [diff] [review]
v1 Remove _needsAlternateEmphasis and alt styling

Try push: https://tbpl.mozilla.org/?tree=Try&noignore=1&rev=a6bef3014f3a
https://tbpl.mozilla.org/?tree=Try&noignore=1&rev=f9cd3dda095b
Attachment #642853 - Flags: review?(bmcbride)
Comment on attachment 642853 [details] [diff] [review]
v1 Remove _needsAlternateEmphasis and alt styling

Review of attachment 642853 [details] [diff] [review]:
-----------------------------------------------------------------

Ever so vaguely concerned about other apps needing that class, but I think the likely hood of them using bold but not using the built-in styles is very slim. Also, less untested code = win.
Attachment #642853 - Flags: review?(bmcbride) → review+
(Assignee)

Comment 3

5 years ago
(In reply to Blair McBride (:Unfocused) from comment #2)
> Comment on attachment 642853 [details] [diff] [review]
> v1 Remove _needsAlternateEmphasis and alt styling
> 
> Review of attachment 642853 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Ever so vaguely concerned about other apps needing that class, but I think
> the likely hood of them using bold but not using the built-in styles is very
> slim. Also, less untested code = win.

I had the same thought process :)  Thought about leaving the alt class there just in case it was needed, but that makes things messier with little (if any) benefit.
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/955bb9f6169c

Try run is in comment 1.
Flags: in-testsuite-
Keywords: checkin-needed
status-firefox16: --- → affected
https://hg.mozilla.org/mozilla-central/rev/955bb9f6169c
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 17
(Assignee)

Comment 6

5 years ago
Comment on attachment 642853 [details] [diff] [review]
v1 Remove _needsAlternateEmphasis and alt styling

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 587909, bug 407861
User impact if declined: Minor - users encountering affected languages will needlessly experience inconsistent highlight styles in results until Firefox 17
Testing completed (on m-c, etc.): Yes, see try/m-i/m-c pushes above
Risk to taking this patch (and alternatives if risky): Low
String or UUID changes made by this patch: None
Attachment #642853 - Flags: approval-mozilla-aurora?
Attachment #642853 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Comment 7

5 years ago
Comment on attachment 642853 [details] [diff] [review]
v1 Remove _needsAlternateEmphasis and alt styling

[Triage Comment]
Fix for a regression in FN16, approved for Aurora 16 since we're still early in the cycle.

Comment 8

5 years ago
(whoops, Gavin already grabbed this)
Keywords: regression
Thanks.
https://hg.mozilla.org/releases/mozilla-aurora/rev/58b2e6a5eae8
status-firefox16: affected → fixed
You need to log in before you can comment on or make changes to this bug.