Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

HTML: ensure BarProps are visible for noopener/noreferrer #16330

Merged
merged 1 commit into from
Apr 18, 2019
Merged

HTML: ensure BarProps are visible for noopener/noreferrer #16330

merged 1 commit into from
Apr 18, 2019

Conversation

annevk
Copy link
Member

@annevk annevk commented Apr 12, 2019

@annevk
Copy link
Member Author

annevk commented Apr 12, 2019

@bzbarsky so I thought this would be an easy way to test whatwg/html#3297, but Chrome passes the test for noopener, so...

@bzbarsky
Copy link
Contributor

Yeah, I have no idea what's going on here. The visual display in Chome is the same whether I pass "locationbar" or "noopener" as the feature string (on Chrome dev 75.0.3759.4), but the barprops return values look different. Seems like a Chrome bug.

@npm1 any idea what's going on here?

@npm1
Copy link
Contributor

npm1 commented Apr 12, 2019

@npm1 any idea what's going on here?

Nope, this is something the DOM team could dig into. You can contact them at their Chromium email: dom-dev.

@annevk
Copy link
Member Author

annevk commented Apr 15, 2019

cc @tkent-google

@tkent-google
Copy link
Contributor

Though I don't know the details, according to crbug.com/310691, Chrome always returns true for BarProp.visible.

@annevk
Copy link
Member Author

annevk commented Apr 15, 2019

@tkent-google I get assert_true: expected true got false for the noreferrer test of this PR.

@tkent-google
Copy link
Contributor

Sorry for my previous useless comment.
I investigated the difference between "noopener" and "noreferer" (currently invalid keyword).
Chrome has the code to propagate parsed window features values to BarProp.visible, and it seems we miss to propagate it in the "noopener" case because it returns null for window.open(). It's an implementation bug definitely.

@annevk
Copy link
Member Author

annevk commented Apr 16, 2019

Okay, so this would show the issue, once that's fixed.

Another thing I could potentially assert is viewport width. That is, requiring it to be identical to the opener. I'm not sure how ideal that is though. Thoughts?

@foolip
Copy link
Member

foolip commented Apr 17, 2019

@tkent-google can I assign this over to you for review? Sounds like you have more context than me. (I was picked at random by the bot.)

@tkent-google tkent-google assigned tkent-google and unassigned foolip Apr 18, 2019
tkent-google
tkent-google approved these changes Apr 18, 2019
View reviewed changes
Copy link
Contributor

@tkent-google tkent-google left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current test doesn't work well only on Chrome due to the Chrome bug, but the test makes sense on other browsers. We fixed the noopener-popup issue of Chrome yesterday, and we have a non-WPT tests. So I think it's ok to merge this as is.

@tkent-google
Copy link
Contributor

I filed the BarProp issue of Chrome; https://bugs.chromium.org/p/chromium/issues/detail?id=954033

@annevk annevk merged commit ce675f0 into master Apr 18, 2019
@annevk annevk deleted the annevk/noopener-noreferrer-and-barprops branch April 18, 2019 07:55
annevk added a commit to whatwg/html that referenced this pull request May 3, 2019
@annevk
Remove noopener/noreferrer from tokenizedFeatures
c0b75ea 
Otherwise CSSOM will assume it's more than a new window and treat it like a popup or some such.

Helps with #1902.

Tests: web-platform-tests/wpt#16330 & web-platform-tests/wpt#16658.
@CYBAI CYBAI mentioned this pull request May 4, 2019
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tkent-google tkent-google tkent-google approved these changes

@domenic domenic Awaiting requested review from domenic

@foolip foolip Awaiting requested review from foolip

@jdm jdm Awaiting requested review from jdm

@jgraham jgraham Awaiting requested review from jgraham

@zcorpan zcorpan Awaiting requested review from zcorpan

@zqzhang zqzhang Awaiting requested review from zqzhang

Assignees

@tkent-google tkent-google

Labels
html
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@annevk @bzbarsky @npm1 @tkent-google @foolip @wpt-pr-bot