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.

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

Ignore `<meta name=viewport>` if `@viewport` is ignored #8538

Merged
merged 1 commit into from Nov 16, 2015

Conversation

@notriddle
Copy link
Contributor

notriddle commented Nov 16, 2015

Fixes #8373

Review on Reviewable

@jdm
Copy link
Member

jdm commented Nov 16, 2015

@jdm
Copy link
Member

jdm commented Nov 16, 2015

We should add an entry to prefs.json, too.

@notriddle
Copy link
Contributor Author

notriddle commented Nov 16, 2015

There already is an entry in prefs.json.

On Sun, Nov 15, 2015, 20:43 Josh Matthews notifications@github.com wrote:

We should add an entry to prefs.json, too.


Reply to this email directly or view it on GitHub
#8538 (comment).

@mbrubeck
Copy link
Contributor

mbrubeck commented Nov 16, 2015

Reviewed 6 of 6 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from the review on Reviewable.io

@mbrubeck
Copy link
Contributor

mbrubeck commented Nov 16, 2015

@bors-servo
Copy link
Contributor

bors-servo commented Nov 16, 2015

📌 Commit 987c9e6 has been approved by mbrubeck

@bors-servo
Copy link
Contributor

bors-servo commented Nov 16, 2015

Testing commit 987c9e6 with merge 2f0475a...

bors-servo added a commit that referenced this pull request Nov 16, 2015
Ignore `<meta name=viewport>` if `@viewport` is ignored

Fixes #8373

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8538)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Nov 16, 2015

💔 Test failed - linux-rel

@mbrubeck
Copy link
Contributor

mbrubeck commented Nov 16, 2015

FAIL [expected PASS] /css21_dev/html4/background-root-101.htm
@jdm
Copy link
Member

jdm commented Nov 16, 2015

@bors-servo
Copy link
Contributor

bors-servo commented Nov 16, 2015

Testing commit 987c9e6 with merge c1e6bc0...

bors-servo added a commit that referenced this pull request Nov 16, 2015
Ignore `<meta name=viewport>` if `@viewport` is ignored

Fixes #8373

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8538)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Nov 16, 2015

@bors-servo bors-servo merged commit 987c9e6 into servo:master Nov 16, 2015
3 checks passed
3 checks passed
code-review/reviewable Review complete: all files reviewed, all discussions resolved
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@frivoal
Copy link

frivoal commented Dec 1, 2015

Hello. Spec co-editor for @Viewport here. Just curious (and confused). What's this about? When do you ignore @Viewport?

@eefriedman
Copy link
Contributor

eefriedman commented Dec 1, 2015

There's a preference which controls whether @viewport rules are processed. The intent is that mobile browsers will enable processing @viewport rules, and desktop browsers will disable it. Web compatibility at its finest. :)

@frivoal
Copy link

frivoal commented Dec 2, 2015

Hmm, that's actually problematic. metia viewport needs to work this way, but @Viewport is explicitely designed so that it would apply to both desktop and mobile and things in between.

There's two points that matter here:

  1. @Viewport can be useful on desktops

  2. not including @Viewport on desktops is harmful

  3. When a site isn't responsive, or only responsive down to a certain size, @Viewport can be used to indicate the smallest acceptable width. @viewport { min-width: 400px; }. Desktop browsers, just like mobile ones, can (and should) respond to that.

  4. If we don't include @Viewport in desktop browsers early on, there's a risk a non trivial amount of website will put something like this in their stylesheet: @viewport { width:320px;}. This will make the site responsive on mobile (even if the number of pixels is slightly wrong for some devices, it's not that far off), but would be terrible on desktops. And if enough sites do that, it becomes impossible to start supporting @Viewport on desktops later, due to backward compat.

What authors should be doing to make their sites responsive is @viewport {width:auto} which is a no-op on desktop since auto is the intial value and it is not overridden by stylesheets, or @viewport {min-width: 320px;} which overrides the 980 px (or whatever) default viewport of mobile browsers, and causes no harm on desktops.

I strongly encourage you to follow microsoft here, and to activate @Viewport on all platform.

On my side, I'll make sure there's a big warning in the spec talking about this. I though there was one already, but I can't find it now, so apparently misremembered.

In the meanwhile, here's a quick and dirty test case that should pass in desktop and mobile browsers alike. (I'll convert that to a clean TC whenever I get some time, unless you want to do so first).

http://output.jsbin.com/gusahiguyu

This passes in Edge.

@frivoal
Copy link

frivoal commented Dec 2, 2015

Specification updated with the relevant warning:
https://drafts.csswg.org/css-device-adapt/#atviewport-rule

@Ms2ger
Copy link
Contributor

Ms2ger commented Dec 2, 2015

FWIW, I think this was one of the features that was behind the catch-all "experimental" flag; it's not clear to me whether the implementation is good enough to enable be default, independent of mobile/desktop. Having a serious test suite for us to run would help, though.

@frivoal
Copy link

frivoal commented Dec 2, 2015

Having a flag for it is good at this stage, but that doesn't change the fact that it should work both on desktop and mobile or neither, not being turned on independently.

As for the test suite, I hear you. I just don't have the resources to focus on it now. If you write some, I will make sure they get reviewed in a timely manner.

@notriddle notriddle deleted the notriddle:github_resize branch Dec 2, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants
You can’t perform that action at this time.