Closed Bug 1246162 Opened 5 years ago Closed 5 years ago

Page content is moving when the "Advanced" button is clicked in "Your connection is not secure" screen

Categories

(Firefox :: General, defect, P1)

All
Unspecified
defect

Tracking

()

VERIFIED FIXED
Firefox 49
Iteration:
49.2 - May 23
Tracking Status
firefox45 --- unaffected
firefox46 --- unaffected
firefox47 + verified
firefox48 + verified
firefox49 + verified

People

(Reporter: pauly, Assigned: nhnt11)

References

()

Details

(Keywords: regression, Whiteboard: [fxprivacy])

Attachments

(2 files, 1 obsolete file)

[Affected versions]:
47.0a1 (2016-02-04)

[Affected platforms]:
Win 7 x64, Ubuntu 14.04 x64, OS X 10.10.5

[Steps to reproduce]:
1. Open https://rc4.badssl.com/
2. Click the "Advanced" button

[Expected result]:
The page content shouldn't be moving when the button is clicked

[Actual result]:
The page content is moving when the button is clicked

[Regression range]:
- regressed by bug 1243310
Whiteboard: [fxprivacy] [triage]
Assignee: nobody → past
Priority: -- → P2
Whiteboard: [fxprivacy] [triage] → [fxprivacy]
Assignee: past → nobody
Priority: P2 → P3
Is this bug still valid? I don't see an Advanced button in FF48 (although it's there in Release).
Flags: needinfo?(past)
RC4 host is SSL_ERROR_NO_CYPHER_OVERLAP in Fx48, but the other error screens still have this problem.

[Tracking Requested - why for this release]:
regression.
Flags: needinfo?(past)
Summary: RC4 page content is moving when the "advanced" button is clicked → Page content is moving when the "Advanced" button is clicked in "Your connection is not secure" screen
Bug 1220481 is changing the page layout and so it might fix this, but in any case I don't think the severity of the problem here warrants tracking.
Depends on: 1220481
Assignee: nobody → nhnt11
Status: NEW → ASSIGNED
Platform triage meeting decision: Tracked for Fx47
Comment on attachment 8748435 [details]
MozReview Request: Bug 1246162 - Position advanced panels absolutely below the main content in about:neterror. r=Gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50277/diff/1-2/
Comment on attachment 8748435 [details]
MozReview Request: Bug 1246162 - Position advanced panels absolutely below the main content in about:neterror. r=Gijs

https://reviewboard.mozilla.org/r/50277/#review47185

Nice!
Attachment #8748435 - Flags: review?(gijskruitbosch+bugs) → review+
Hi Nihanth, once this patch lands in Nightly, could you please nominate for uplift to Beta/Aurora?
Flags: needinfo?(nhnt11)
https://hg.mozilla.org/integration/fx-team/rev/335e95e6cdc528b60638b2a9802071f8143fe854
Bug 1246162 - Position advanced panels absolutely below the main content in about:neterror. r=Gijs
Comment on attachment 8748435 [details]
MozReview Request: Bug 1246162 - Position advanced panels absolutely below the main content in about:neterror. r=Gijs

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]: UI jumps when advanced button is clicked, bad experience.
[Describe test coverage new/current, TreeHerder]: none
[Risks and why]: Low risk. Simple UI change, tested.
[String/UUID change made/needed]: none
Flags: needinfo?(nhnt11)
Attachment #8748435 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/335e95e6cdc5
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Blocks: 1216897
Iteration: --- → 49.2 - May 23
Flags: qe-verify?
Priority: P3 → P1
Comment on attachment 8748435 [details]
MozReview Request: Bug 1246162 - Position advanced panels absolutely below the main content in about:neterror. r=Gijs

Let's uplift to both Aurora48 and Beta47
Attachment #8748435 - Flags: approval-mozilla-beta+
Attachment #8748435 - Flags: approval-mozilla-aurora?
Attachment #8748435 - Flags: approval-mozilla-aurora+
Hello Nihanth, for test coverage in the absence of automated tests, could you please mention whether you manually verified the fix or not?
Flags: needinfo?(nhnt11)
Hi Pauly, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!
Flags: needinfo?(paul.silaghi)
This isn't applying cleanly to beta. Can we get a rebased patch if this is needed for 47?
Comment on attachment 8748435 [details]
MozReview Request: Bug 1246162 - Position advanced panels absolutely below the main content in about:neterror. r=Gijs

I didn't request approval for uplift to beta because bug 1220481 (on which this depends) landed in 48. I can't see a way to remove the beta approval flag.

This patch needs to be backported for landing on 47, would you take a completely separate/backported patch for beta? I will work on it if so.
Flags: needinfo?(nhnt11) → needinfo?(rkothari)
Er, to answer your question: yes, this patch was manually tested locally.
(In reply to Nihanth Subramanya [:nhnt11] from comment #17)
> Comment on attachment 8748435 [details]
> MozReview Request: Bug 1246162 - Position advanced panels absolutely below
> the main content in about:neterror. r=Gijs
> 
> I didn't request approval for uplift to beta because bug 1220481 (on which
> this depends) landed in 48. I can't see a way to remove the beta approval
> flag.
> 
> This patch needs to be backported for landing on 47, would you take a
> completely separate/backported patch for beta? I will work on it if so.

Oops, my bad. I think we should be fixing this for Fx47. Yes, let's work on a different patch for beta if that is what is needed to fix this. We can evaluate the risk of the beta patch and then decide whether to uplift to Beta or not.
Flags: needinfo?(rkothari)
Attached patch Patch for beta (obsolete) — Splinter Review
Patch for beta. This does not touch the cert error page, which doesn't suffer from page jumping but instead suffers from the main content being positioned as if the advanced panel is always visible (i.e. as if the advanced panel was set to visibility: hidden rather than display: none). See bug 1247176.
Attachment #8751013 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8751013 [details] [diff] [review]
Patch for beta

Review of attachment 8751013 [details] [diff] [review]:
-----------------------------------------------------------------

Does this not need the padding fix on the container to replace the margins on the #weakCryptoAdvanced panel, then? I'm assuming you've tested this, and assuming that it really doesn't need that change, r=me.
Attachment #8751013 - Flags: review?(gijskruitbosch+bugs) → review+
(In reply to Paul Silaghi, QA [:pauly] from comment #0)
> [Regression range]:
> - regressed by bug 1243310
This fix makes bug 1243310 occur again.
Flags: needinfo?(paul.silaghi) → needinfo?(nhnt11)
(In reply to Paul Silaghi, QA [:pauly] from comment #23)
> (In reply to Paul Silaghi, QA [:pauly] from comment #0)
> > [Regression range]:
> > - regressed by bug 1243310
> This fix makes bug 1243310 occur again.

I can't reproduce this on Nightly. What specifically do you mean - can you provide steps and a screenshot? The conclusion in bug 1243310 is that the "advanced" pane *not* taking up the full width of the rest of the page, AND the existing content not shifting, is the correct behaviour. On my current OS X nightly, that is exactly what happens.
Flags: needinfo?(nhnt11) → needinfo?(paul.silaghi)
49.0a1 (2016-05-11) Win 7 x64:
http://i.imgur.com/MfNDkfQ.png
http://i.imgur.com/3S6WfkR.png

(In reply to :Gijs Kruitbosch from comment #24)
> The conclusion in bug 1243310 is that the
> "advanced" pane *not* taking up the full width of the rest of the page, AND
> the existing content not shifting, is the correct behaviour.
I thought this was the bad behavior and was fixed by bug 1243310 - just checked a nightly 47.0a1 (2016-02-02) after bug 1243310 was fixed
Flags: needinfo?(paul.silaghi)
(In reply to Paul Silaghi, QA [:pauly] from comment #25)
> 49.0a1 (2016-05-11) Win 7 x64:
> http://i.imgur.com/MfNDkfQ.png
> http://i.imgur.com/3S6WfkR.png

It's still not clear to me what you think is wrong in these screenshots. Please be more specific.
Flags: needinfo?(paul.silaghi)
The width of the expanded advanced section is too small - http://i.imgur.com/ZIpmJ9L.png.
It should be bigger and have a fixed width, just like before the fix of this bug landed.
Flags: needinfo?(paul.silaghi)
(In reply to Paul Silaghi, QA [:pauly] from comment #27)
> The width of the expanded advanced section is too small -
> http://i.imgur.com/ZIpmJ9L.png.
> It should be bigger and have a fixed width, just like before the fix of this
> bug landed.

I don't know that that's true, but let's ask other folks. Either way - that's a separate bug from bug 1243310 which was about the alignment, not the width, of the items...
Flags: needinfo?(nhnt11)
Flags: needinfo?(bram)
Yeah, this is a regression, the advanced panel was supposed to take the full width of the content column. It seems position: absolute; causes this, a fix is to add width: 100% to the container. Unfortunate regression. I tested the patch with a page that has a lot of advanced panel content (so that the it occupies the full width) since that's when the jump was significant. I'll file a bug...
Flags: needinfo?(nhnt11)
Flags: needinfo?(bram)
Depends on: 1272531
(In reply to Nihanth Subramanya [:nhnt11] from comment #29)
> Yeah, this is a regression, the advanced panel was supposed to take the full
> width of the content column. It seems position: absolute; causes this, a fix
> is to add width: 100% to the container. Unfortunate regression. I tested the
> patch with a page that has a lot of advanced panel content (so that the it
> occupies the full width) since that's when the jump was significant. I'll
> file a bug...

Thanks. Once you have a fix there can you update the beta patch here and resubmit that for review so we can get this on beta ASAP?
Added the width: 100% and a bottom padding (I found a case where the bottom padding is needed, probably a good idea to throw it in).
Attachment #8751013 - Attachment is obsolete: true
Attachment #8752355 - Flags: review?(gijskruitbosch+bugs)
Attachment #8752355 - Flags: review?(gijskruitbosch+bugs) → review+
Hello Nihanth, is the new rebased beta patch ready? I'd like to uplift it in time for 47.0b7 Thursday 5/19.
Flags: needinfo?(nhnt11)
Comment on attachment 8752355 [details] [diff] [review]
Patch for beta v2

Bah, forgot to request approval, sorry :(

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]: UI jumps when advanced button is clicked, bad experience.
[Describe test coverage new/current, TreeHerder]: none
[Risks and why]: Low risk. Simple UI change, tested.
[String/UUID change made/needed]: none
Flags: needinfo?(nhnt11)
Attachment #8752355 - Flags: approval-mozilla-beta?
Hi Paul, could you please verify this issue is fixed as expected on 47.0b7 (hoping the fix will land by then)? Thanks!
Flags: needinfo?(paul.silaghi)
Comment on attachment 8752355 [details] [diff] [review]
Patch for beta v2

Recent P1 regression, Beta47+
Attachment #8752355 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify? → qe-verify+
QA Contact: paul.silaghi
The advanced panel takes the full width of the content column.
Verified fixed FX 47b7 Win 7, Ubuntu 14.04, OS X 10.10.5.
Flags: needinfo?(paul.silaghi)
Also marking this as verified on FX 48.0a2, 49.0a1 considering the follow-up bug 1272531.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.