Closed Bug 1186892 Opened 6 years ago Closed 5 years ago

Strikethrough of "http" appears broken when protection is disabled on pages with insecure content

Categories

(Firefox :: Security, defect, P4)

42 Branch
All
Windows 7
defect

Tracking

()

VERIFIED FIXED
Firefox 50
Tracking Status
firefox42 --- wontfix
firefox47 --- wontfix
firefox48 --- wontfix
firefox49 + wontfix
firefox50 + verified

People

(Reporter: VarCat, Assigned: xidorn)

References

Details

(Whiteboard: [fxprivacy])

Attachments

(2 files)

Attached image visual_bug.jpg
Evironment:

Win 7 x64
FF 42
Build Id: 20150722030205

STR:

1. Go to https://www.bennish.net/mixed-content.html
2. Disable protection for insecure content.

Issue:
The strikethrough over http is interrupted. For a better explanation of the bug please check the attached screenshot.
Moving this to Firefox :: Security to match the parent bug.
Component: General → Security
Summary: [Windows]Strikethrough visual bug for pages that have insecure content loaded → Strikethrough of "http" appears broken when protection is disabled on pages with insecure content
Whiteboard: [fxprivacy]
Whiteboard: [fxprivacy] → [fxprivacy] [triage]
Priority: -- → P4
Whiteboard: [fxprivacy] [triage] → [fxprivacy]
Blocks: 1216897
This bug is still reproducible on 50.0a1 (2016-06-09) Win 7
Xidorn, do you know if this could be related to what you fixed in Bug 1277937?
Flags: needinfo?(bugzilla)
Not sure what's wrong here. Is the issue about that the strikethrough is a dashed line rather than a solid gray line?

That is an issue unrelated to bug 1277937. And it seems the code for this doesn't really consider the use of selection decoration here. Could anyone tell me what is the expected effect? Then I can probably fix this.
Flags: needinfo?(bugzilla)
(In reply to Xidorn Quan [:xidorn] (UTC+1) from comment #5)
> Not sure what's wrong here. Is the issue about that the strikethrough is a
> dashed line rather than a solid gray line?
Yes
The https should have a full line strikethrough instead of the dashes.  I'm not sure what caused this regression, but it seems like it has been around for a while and we need to fix it.
Note that it only reproduces on Windows.
It seems to me the whole selection decoration mechanism in nsTextFrame never considers the URLStrikeout selection type. I guess it was initially only used for IME. And consequently you can see lots of warnings printed to output about invalid underline style when the strike line there is displayed.
The selection decoration code looks pretty broken with the additional URL strikeout... But given that is only used internally, and in only one place, I guess it isn't necessary to fix everything now... I think it does need some cleanup eventually, though...
Tracking to make sure this lands. We could also consider uplifting the fix.
Comment on attachment 8763415 [details]
Bug 1186892 - Handle url strikeout separately from IME selection types.

https://reviewboard.mozilla.org/r/59578/#review56704

Yes, this makes sense AFAICS.
Attachment #8763415 - Flags: review?(jfkthame) → review+
Assignee: nobody → bugzilla
Keywords: checkin-needed
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #12)
> Tracking to make sure this lands. We could also consider uplifting the fix.

I would rather not uplift this fix. This bug is a long time and minor issue, not a recent regression. The change here isn't that trivial, especially given that the related code seems broken to me, and I don't believe it has enough coverage from automatic tests, so I suppose there exists certain risk.
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #14)
> (In reply to Liz Henry (:lizzard) (needinfo? me) from comment #12)
> > Tracking to make sure this lands. We could also consider uplifting the fix.
> 
> I would rather not uplift this fix. This bug is a long time and minor issue,
> not a recent regression. The change here isn't that trivial, especially
> given that the related code seems broken to me, and I don't believe it has
> enough coverage from automatic tests, so I suppose there exists certain risk.

Yeah, I'm pretty sure not uplifting is fine with fxprivacy
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/96070b513deb
Handle url strikeout separately from IME selection types. r=jfkthame
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/96070b513deb
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
Flags: qe-verify+
QA Contact: paul.silaghi
Verified fixed FX 50.0a1 (2016-06-21) Win 7
Status: RESOLVED → VERIFIED
Hi Xidorn
Since this patch also affects 49, are you also considering to uplift this patch to 49?
Flags: needinfo?(xidorn+moz)
Given comment 14 (and comment 15), I'm not going to uplift this patch.
Flags: needinfo?(xidorn+moz)
Per comment #14 & #15, the bug has been there for a while. Mark 49 as won't fix.
You need to log in before you can comment on or make changes to this bug.