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

Status: RESOLVED FIXED

(Firefox :: Address Bar, defect)

Description

[Andrew Hurle [:ahurle]]

Attachment: 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.

Comment 1

[Andrew Hurle [:ahurle]]

Attachment: 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

Comment 2

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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.

Comment 3

[Andrew Hurle [:ahurle]]

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

Comment 4

[Matthew N. [:MattN]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/955bb9f6169c

Try run is in comment 1.

Comment 5

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/955bb9f6169c

Comment 6

[Andrew Hurle [:ahurle]]

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

Comment 7

[Alex Keybl [:akeybl]]

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

[Alex Keybl [:akeybl]]

(whoops, Gavin already grabbed this)

Comment 9

[Matthew N. [:MattN]]

Thanks.
https://hg.mozilla.org/releases/mozilla-aurora/rev/58b2e6a5eae8