Last Comment Bug 774495 - Matches containing ligatures in awesomebar results should not be underlined, use normal style instead
: Matches containing ligatures in awesomebar results should not be underlined, ...
Status: RESOLVED FIXED
: regression
Product: Firefox
Classification: Client Software
Component: Location Bar (show other bugs)
: Trunk
: All All
: -- minor (vote)
: Firefox 17
Assigned To: Andrew Hurle [:ahurle]
:
Mentors:
Depends on: 587909
Blocks:
  Show dependency treegraph
 
Reported: 2012-07-16 15:40 PDT by Andrew Hurle [:ahurle]
Modified: 2012-07-23 14:02 PDT (History)
5 users (show)
MattN+bmo: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
Screenshot showing current emphasis on the left, desired change on the right (5.76 KB, image/png)
2012-07-16 15:40 PDT, Andrew Hurle [:ahurle]
no flags Details
v1 Remove _needsAlternateEmphasis and alt styling (5.09 KB, patch)
2012-07-16 20:48 PDT, Andrew Hurle [:ahurle]
bmcbride: review+
gavin.sharp: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Andrew Hurle [:ahurle] 2012-07-16 15:40:36 PDT
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.
Comment 1 Andrew Hurle [:ahurle] 2012-07-16 20:48:01 PDT
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
Comment 2 Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not) 2012-07-17 06:16:41 PDT
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] 2012-07-17 16:04:48 PDT
(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] 2012-07-18 01:06:45 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/955bb9f6169c

Try run is in comment 1.
Comment 5 Ed Morley [:emorley] 2012-07-18 05:52:39 PDT
https://hg.mozilla.org/mozilla-central/rev/955bb9f6169c
Comment 6 Andrew Hurle [:ahurle] 2012-07-19 15:08:36 PDT
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] 2012-07-23 11:31:54 PDT
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] 2012-07-23 11:32:27 PDT
(whoops, Gavin already grabbed this)
Comment 9 Matthew N. [:MattN] 2012-07-23 14:02:38 PDT
Thanks.
https://hg.mozilla.org/releases/mozilla-aurora/rev/58b2e6a5eae8

Note You need to log in before you can comment on or make changes to this bug.