Closed Bug 1277937 Opened 4 years ago Closed 4 years ago

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

Categories

(Firefox :: General, defect, P1)

48 Branch
defect

Tracking

()

VERIFIED FIXED
Firefox 50
Iteration:
50.1 - Jun 20
Tracking Status
firefox46 --- unaffected
firefox47 --- unaffected
firefox48 + verified
firefox49 --- verified
firefox-esr45 --- unaffected
firefox50 --- verified

People

(Reporter: tanvi, Assigned: xidorn)

References

Details

(Keywords: regression, Whiteboard: [fxprivacy])

Attachments

(2 files)

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
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!
Flags: needinfo?(bugzilla)
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.
Yes, I mean the code. Thanks for the information.
Taking this. Will submit patch soon, waiting for local build.
Assignee: nobody → bugzilla
Flags: needinfo?(bugzilla)
Version: 47 Branch → 48 Branch
When do we show the struck through https?  Are there any cases other than when mixed active content has loaded?
Attachment #8760041 - Flags: review?(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 on attachment 8760041 [details]
Bug 1277937 - Fix SELECTION_URLSTRIKEOUT.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/57770/diff/1-2/
Attachment #8760041 - Flags: review?(jfkthame)
Attachment #8760041 - Flags: review?(jfkthame) → review+
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.
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.
Whiteboard: [fxprivacy][triage]
Keywords: checkin-needed
Pushed by mozilla@noorenberghe.ca:
https://hg.mozilla.org/integration/fx-team/rev/2666d8ddb8ef
Fix SELECTION_URLSTRIKEOUT. r=jfkthame
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/2666d8ddb8ef
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
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.
Flags: qe-verify+
Flags: needinfo?(florin.mezei)
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
Attachment #8760041 - Flags: approval-mozilla-beta?
Attachment #8760041 - Flags: approval-mozilla-aurora?
Flags: needinfo?(florin.mezei)
QA Contact: paul.silaghi
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.
Status: RESOLVED → VERIFIED
Whiteboard: [fxprivacy]
Comment on attachment 8760041 [details]
Bug 1277937 - Fix SELECTION_URLSTRIKEOUT.

Regression from 48, please uplift to aurora and beta.
Attachment #8760041 - Flags: approval-mozilla-beta?
Attachment #8760041 - Flags: approval-mozilla-beta+
Attachment #8760041 - Flags: approval-mozilla-aurora?
Attachment #8760041 - Flags: approval-mozilla-aurora+
Iteration: --- → 50.1
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
Flags: needinfo?(bugzilla)
Verified fixed FX 49.0a2 (2016-06-23), 48b3 Win 7.
You need to log in before you can comment on or make changes to this bug.