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
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!
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.
See my attached image
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
Yes, I mean the code. Thanks for the information.
Taking this. Will submit patch soon, waiting for local build.
Review commit: https://reviewboard.mozilla.org/r/57770/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/57770/
When do we show the struck through https? Are there any cases other than when mixed active content has loaded?
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) → 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.
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/fx-team/rev/2666d8ddb8ef Fix SELECTION_URLSTRIKEOUT. r=jfkthame
4 years ago
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 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
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.
Comment on attachment 8760041 [details] Bug 1277937 - Fix SELECTION_URLSTRIKEOUT. Regression from 48, please uplift to aurora and beta.
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
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
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.