Mike Taylor [:miketaylr] Reporter
	Description • 7 years ago

Originally reported @ https://webcompat.com/issues/10152

Steps to reproduce:

1) install Elm lang (https://guide.elm-lang.org/install.html)
2) Clone https://github.com/evancz/elm-architecture-tutorial.git
3) Run elm reactor `elm reactor`, it'll start a local server
4) Open http://localhost:8000/elm-architecture-tutorial/examples/01-button.elm   5) Click on "Explore History" in bottom right.

Expected:

There's a little editor in the pop-up window

Actual:

The pop-up is blank.


mozregression points here:

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=7e5b28819e5b382bea8cbcccf0c6402fe0e90192&tochange=0533dbf2956818762fbbe7dbcfad738fe8383d89

	Florian Quèze [:florian] Assignee
	Comment 3 • 7 years ago

This ignores about:blank paints only when the body is empty. My first idea was to check for a non-empty rect on the MozAfterPaint event, but that didn't work as the painted rect isn't empty for the normal about:blank.

	Florian Quèze [:florian] Assignee
	Comment 4 • 7 years ago

With this patch applied, there's an edge case that remains where we would not paint about:blank: if a page opens an empty popup, the opacity will be 0 until some JS on the opener actually changes something to the document.

This would only be noticeable if the about:blank color is significantly different from the tabbrowser background. That means in private browsing windows, or if the user customized the default page background color in the preferences.

I can't think of a reason why this behavior difference would matter, as the popup window would be empty anyway, the only difference is the color it will be filled with... until some content is actually added to it.

	Florian Quèze [:florian] Assignee
	Comment 5 • 7 years ago

Mike, is there any chance you could test the patch I attached here? Would you like me to push it to try to generate builds or can you easily apply it to a local build?

	Dão Gottwald [:dao]
	Comment 6 • 7 years ago

(In reply to Florian Quèze [:florian] [:flo] from comment #4)
> With this patch applied, there's an edge case that remains where we would
> not paint about:blank: if a page opens an empty popup, the opacity will be 0
> until some JS on the opener actually changes something to the document.

It sounds like you really want to check content.opener rather than content.document.body.hasChildNodes().

	Mike Taylor [:miketaylr] Reporter
	Comment 7 • 7 years ago

Yep, works here!

	Florian Quèze [:florian] Assignee
	Comment 8 • 7 years ago

(In reply to Dão Gottwald [::dao] from comment #6)

> It sounds like you really want to check content.opener rather than
> content.document.body.hasChildNodes().

Good idea, thanks!

	Dão Gottwald [:dao]
	Comment 9 • 7 years ago

Comment on attachment 8912787 [details] [diff] [review]
Patch v2

I also tested this locally

	Pulsebot
	Comment 10 • 7 years ago

Pushed by florian@queze.net:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f5d781a4d148
ignore about:blank paints only when window.opener isn't set, r=dao.

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 11 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/f5d781a4d148

	Ryan VanderMeulen [:RyanVM]
	Comment 12 • 7 years ago

Please request Beta approval on this when you get a chance.

	Florian Quèze [:florian] Assignee
	Comment 13 • 7 years ago

Comment on attachment 8912787 [details] [diff] [review]
Patch v2

Approval Request Comment
[Feature/Bug causing the regression]: bug 1379587
[User impact if declined]: Some popups will be blank (when a websites opens an about:blank popup and then controls its content with JavaScript in the opener window)
[Is this code covered by automated tests?]: No. I couldn't find a simple way to write an automated test that isn't just reproducing implementation details of using the blank attribute. So such a test wouldn't really prevent regressions.
[Has the fix been verified in Nightly?]: No. But the patch has been verified by the reporter in comment 7.
[Needs manual test from QE? If yes, steps to reproduce]: While I'm confident in the fix for this specific edge case, I think more QA on bug 1379587 wouldn't hurt.
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: just adding a simple check that disables an optimization in a case where it had an undesirable behavior.
[String changes made/needed]: none

	Sylvestre Ledru [:Sylvestre]
	Comment 14 • 7 years ago

Fix a recent regression, tracking it.

	Sylvestre Ledru [:Sylvestre]
	Comment 15 • 7 years ago

Comment on attachment 8912787 [details] [diff] [review]
Patch v2

taking it to 57b4.

	Ryan VanderMeulen [:RyanVM]
	Comment 16 • 7 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-beta/rev/81bcbd6fa446

	Mihai Boldan, Desktop QA [:mboldan]
	Comment 17 • 7 years ago

I managed to reproduce this issue on Firefox 58.0a1 (2017-09-26), under Windows 10x64.
The issue is not longer reproducible on Firefox 57.0b11, or on Firefox 58.0a1 (2017-10-26).
Tests were executed under Windows 10x64 and under Mac OS X 10.10.5.

Note that I was not able to test this on Ubuntu OS since I encountered some issues trying to install the elm-reactor. 
Please let me know if it's necessary confirming this issue on Ubuntu as well. 
I will remove the qe-verify flag only after the confirmation related to the Ubuntu OS.

	noahloomans
	Comment 18 • 7 years ago

Hello,

I originally reported this bug. I am able to confirm that this is fixed in Ubuntu.

Thank you all so much for fixing this bug in such a timely manner after I reported in, I really appreciate it and am loving my experience with Firefox Nightly. Looking forward to Firefox 57 being officially released!

Greeting,

Noah Loomans.