1277937 - https has an underline instead of a strikethrough when mixed active content is loaded

Status: VERIFIED FIXED

(Firefox :: General, defect, P1)

Description

[Tanvi Vyas[:tanvi]]

STR:
1) Go to https://people.mozilla.org/~tvyas/mixedcontent.html
2) Open the control center, go to the submenu.
3) Click Disable Protection
4) observe the url bar.  It should show an crossed out grey lock and a crossed out https, like the fourth picture in the second column here:
https://ffp4g1ylyit3jdyti1hqcvtb-wpengine.netdna-ssl.com/security/files/2015/10/combo-graph21.png.  It actually looks like a crossed out lock with an *underlined https*.

Why did the https switch from being struck through to underlined?  What release did this issue occur in?  Sometime between 46 and 49.  In 46 it looks good, in 49 it does not.




+++ This bug was initially created as a clone of Bug #1253771 +++

(In reply to Tanvi Vyas - behind on reviews [:tanvi] from comment #21)
> (In reply to Johann Hofmann [:johannh] from comment #18)
> > (In reply to agrigas from comment #17)
> > > Also the https shouldnt have a line under it...
> > 
> > When I visit a site like https://mixed-script.badssl.com/ and turn off
> > protection then it has that line on my Aurora/DevEdition and Nightly, but
> > not on stable Firefox. Is that a bug? We should really fix it then.
> 
> That seems like a bad regression!  We do not want to underline https when
> protection is disabled!  It is supposed to be struck through.  On Firefox 46
> release, I see a struck through https in the url bar when we disable
> protection.  But on Nightly, I see an underlined https in the url bar.  That
> is no good!
> 
> https://people.mozilla.com/~tvyas/mixedcontent.html

Comment 1

[Kamil Jozwiak [:kjozwiak]]

Found the following regression:

9:56.49 INFO: Last good revision: b79f70a20d4a380715f1fc79a986aac6703d993a
9:56.49 INFO: First bad revision: e4d32bd5e11e3612f91cea4fc8fd799019304351
9:56.49 INFO: Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=b79f70a20d4a380715f1fc79a986aac6703d993a&tochange=e4d32bd5e11e3612f91cea4fc8fd799019304351

9:57.76 INFO: Looks like the following bug has the changes which introduced the regression:
https://bugzilla.mozilla.org/show_bug.cgi?id=1258636

Xidorn, would you mind taking a look whenever you have some time? Thanks!

Comment 2

[Xidorn Quan [:xidorn] UTC+10]

Could someone tell me what style does it use to display that line? I had a look via the browser toolbox inspector, but couldn't find anything useful.

Comment 3

[Johann Hofmann [:johannh]]

Attachment: underlined https

See my attached image

Comment 4

[Johann Hofmann [:johannh]]

Oh, were you referring to the code that draws that line? You can find it here: https://dxr.mozilla.org/mozilla-central/source/browser/base/content/urlbarBindings.xml#216

Comment 5

[Xidorn Quan [:xidorn] UTC+10]

Yes, I mean the code. Thanks for the information.

Comment 6

[Xidorn Quan [:xidorn] UTC+10]

Taking this. Will submit patch soon, waiting for local build.

Comment 7

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1277937 - Fix SELECTION_URLSTRIKEOUT.

Review commit: https://reviewboard.mozilla.org/r/57770/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/57770/

Comment 8

[Tanvi Vyas[:tanvi]]

When do we show the struck through https?  Are there any cases other than when mixed active content has loaded?

Comment 9

[Jonathan Kew [:jfkthame]]

Comment on attachment 8760041 [details]
Bug 1277937 - Fix SELECTION_URLSTRIKEOUT.

https://reviewboard.mozilla.org/r/57770/#review54632

::: layout/generic/nsTextFrame.cpp:5616
(Diff revision 1)
> -      offset = metrics.strikeoutOffset + 0.5;
> -      aDecoration = NS_STYLE_TEXT_DECORATION_LINE_LINE_THROUGH;
> +      params.offset = metrics.strikeoutOffset + 0.5;
> +      params.decoration = NS_STYLE_TEXT_DECORATION_LINE_LINE_THROUGH;

It looks like this fail to account for aDecorationOffsetDir in the resulting params.offset, doesn't it?

Comment 10

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8760041 [details]
Bug 1277937 - Fix SELECTION_URLSTRIKEOUT.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/57770/diff/1-2/

Comment 11

[Jonathan Kew [:jfkthame]]

Comment on attachment 8760041 [details]
Bug 1277937 - Fix SELECTION_URLSTRIKEOUT.

https://reviewboard.mozilla.org/r/57770/#review54636

Looks better, thanks. Could we have reftests for this, too? We could at least verify that text-decoration: none / underline / overline / line-through all render differently.

Comment 12

[Xidorn Quan [:xidorn] UTC+10]

That wouldn't catch this regression either.

I guess it might be good to open another bug and have a mochitest to check behavior of decoration lines.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/2666d8ddb8ef
Fix SELECTION_URLSTRIKEOUT. r=jfkthame

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2666d8ddb8ef

Comment 15

[Johann Hofmann [:johannh]]

We will probably need QA verification for this. Florin, could you take a look? It's a bit urgent, since this regression is already in Beta. STR see first comment.

Comment 16

[Johann Hofmann [:johannh]]

Comment on attachment 8760041 [details]
Bug 1277937 - Fix SELECTION_URLSTRIKEOUT.

Approval Request Comment
[Feature/regressing bug #]: Bug 1258636
[User impact if declined]: Pretty serious Security UI regression, all stroke through https signs for insecure pages will instead be underlined.

[Describe test coverage new/current, TreeHerder]: See comments, this is a visual regression that would not be easy to catch by a machine but can easily be verified by QA. We're waiting on QA right now.

[Risks and why]: The change is really small, seems to be just correcting an oversight from Bug 1258636. If anything were wrong, we might introduce another visual regression.

[String/UUID change made/needed]: none

Comment 17

[Paul Silaghi, QA [:pauly]]

Verified fixed FX 50.0a1 (2016-06-08) Win 7, Ubuntu 16.04, OS X 10.11.
Fyi, bug 1186892 is related to this and it's still reproducible on Windows.

Comment 18

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8760041 [details]
Bug 1277937 - Fix SELECTION_URLSTRIKEOUT.

Regression from 48, please uplift to aurora and beta.

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/109773972727

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/7b71e7abae78

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out from beta because it apparently made some android reftests near permafail:

https://hg.mozilla.org/releases/mozilla-beta/rev/b449af6b8741

https://treeherder.mozilla.org/#/jobs?repo=mozilla-beta&bugfiler&fromchange=331c922bff8152bc967e9065a97566f9f712e434&filter-tier=1&selectedJob=1190227&filter-searchStr=6c23eb02ba6973e0b2514fcc333c02202c2e5f59

Comment 22

[Xidorn Quan [:xidorn] UTC+10]

It has been clear that permafail isn't this patch's fault, and it has been re-land on beta by philor:
https://hg.mozilla.org/releases/mozilla-beta/rev/f751465bac6a

Comment 23

[Paul Silaghi, QA [:pauly]]

Verified fixed FX 49.0a2 (2016-06-23), 48b3 Win 7.