Support dynamic viewport changes (changing meta viewport tag from JavaScript)

RESOLVED FIXED in Firefox 43

Status

()

RESOLVED FIXED
5 years ago
5 months ago

People

(Reporter: miketaylr, Assigned: miketaylr, Mentored)

Tracking

(Depends on: 1 bug, Blocks: 1 bug, {dev-doc-complete})

unspecified
Firefox 43
All
Android
dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox43 fixed)

Details

(Whiteboard: [lang=js][bad first bug], URL)

Attachments

(4 attachments, 7 obsolete attachments)

1.30 KB, patch
miketaylr
: review+
Details | Diff | Splinter Review
4.30 KB, patch
miketaylr
: review+
Details | Diff | Splinter Review
2.63 KB, patch
miketaylr
: review+
Details | Diff | Splinter Review
5.85 KB, patch
miketaylr
: review+
Details | Diff | Splinter Review
Certain mobile sites on the web assume that you can dynamically change meta viewport content (and have it actually do something).

Currently we don't support this, but only allow changing the viewport if you append a new meta element with a different value.

See https://miketaylr.com/bzla/viewport-change.html for a test case. 

Expected: Clicking all 4 buttons changes the viewport
Actual: Only the append buttons work
See Also: → bug 784463
Thanks for filing this, Mike!

Copying over my comment from bug 784463:

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #15)
> I thought we did support that but looking at our code it seems we only
> listen for new meta elements being added, not removal of meta elements or
> modifications of existing meta elements. The code around [1] probably needs
> to be updated to listen for more events.
> 
> [1]
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/
> browser.js?rev=267e352db60c#5803

I can mentor this bug if there's anybody interested in fixing it.
OS: Mac OS X → Android
Hardware: x86 → All
Whiteboard: [mentor=kats][lang=js][bad first bug]
Mentor: bugmail.mozilla
Whiteboard: [mentor=kats][lang=js][bad first bug] → [lang=js][bad first bug]
Duplicate of this bug: 1061958
Updating title to make it easier to find.
Summary: Support dynamic viewport changes → Support dynamic viewport changes (changing meta viewport tag from JavaScript)
Unsure if this should be duped against bug 617331 or bug 714737 or what. I just stumbled upon those.
Duplicate of this bug: 617331
Duplicate of this bug: 714737
I duped them here since this bug had more info and is mentored. Note however that Matt provides a workaround at https://bugzilla.mozilla.org/show_bug.cgi?id=714737#c3
Keywords: dev-doc-needed

Comment 8

4 years ago
Just want to state the obvious. Button 3 and 4 of Mike's test page will also work even if the 'older' meta tag is removed before or after the append action.
I've got some local patches that have this working, will try to write some tests and post 'em in the next few days.
Assignee: nobody → miket
Thanks! If you have patches I'd love to see them. In bug 1180267 I'm in the process of ripping out some of the code that you might have touched depending on your implementation.
Here you go -- nothing fancy, just connecting the dots between HTMLMetaElement::AfterSetAttr and nsContentUtils::ProcessViewportInfo, and then adding a fallthrough for DOMMetaChanged into the DOMMetaAdded condition of ViewPortHandler.handleEvent. 

Looking at your patches in 1180267 it seems like there shouldn't be any conflicts.
Attachment #8651519 - Flags: feedback?(bugmail.mozilla)
Comment on attachment 8651519 [details] [diff] [review]
976616.-Support-dynamic-viewport-changes.-WIP.patch

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

Cool, thanks! So the only changes that you'll need are to also listen for the DOMMetaChanged event in layout/base/MobileViewportManager. With my patches applied the viewport computation is no longer done in browser.js but is instead done in MobileViewportManager, same as for all our other platforms. You'll still need the DOMMetaChanged in browser.js because the zoom constraints calculation is still (for now) done there.
Attachment #8651519 - Flags: feedback?(bugmail.mozilla) → feedback+
Depends on: 1180267
bholley, do you mind reviewing the changes to HTMLMetaElement?
Attachment #8651519 - Attachment is obsolete: true
Attachment #8651989 - Flags: review?(bobbyholley)
Attachment #8651990 - Flags: review?(bugmail.mozilla) → review+
Comment on attachment 8651991 [details] [diff] [review]
976616.-Part-3-Enable-meta-viewport-tests-for-Fe.patch

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

I don't think this does the right thing. For fennec, for example, toolkit==android but buildapp!=b2g, so the skip-if condition will be true, and the test will be skipped. In fact I think this will skip it in more places than it was skipped before.
Attachment #8651991 - Flags: review?(bugmail.mozilla) → review-
I'm confused - why do we have a viewport attribute on media elements at all?

The answer could very well be "no good reason but we need this for webcompat", but I'd like to understand what motivated us to do this in the first place.
Oh sorry, I read HTMLMediaElement instead of HTMLMetaElement. Don't mind me!
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #17)
> Comment on attachment 8651991 [details] [diff] [review]
> 976616.-Part-3-Enable-meta-viewport-tests-for-Fe.patch
> 
> Review of attachment 8651991 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I don't think this does the right thing. For fennec, for example,
> toolkit==android but buildapp!=b2g, so the skip-if condition will be true,
> and the test will be skipped. In fact I think this will skip it in more
> places than it was skipped before.

Ah, right. Let me take a closer look at mochitest conditional logic-fu.
Attachment #8651989 - Flags: review?(bobbyholley) → review+
(In reply to Bobby Holley (:bholley) from comment #19)
> Oh sorry, I read HTMLMediaElement instead of HTMLMetaElement. Don't mind me!

:)~
Comment on attachment 8651992 [details] [diff] [review]
976616.-Part-4-Add-test-for-dynamic-meta-viewpor.patch

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

Looks good, thanks!

::: dom/base/test/test_meta_viewport7.html
@@ +101,5 @@
> +      meta.content = content;
> +    }
> +
> +    function nextTest() {
> +      if (tests.length)

braces please (i realize this is copy-paste, but we can update the style a bit :))
Attachment #8651992 - Flags: review?(bugmail.mozilla) → review+
Adding r=bholley to commit message and carrying forward r+.
Attachment #8651989 - Attachment is obsolete: true
Attachment #8652031 - Flags: review+
Adding r=kats to commit msg and carrying forward r+.
Attachment #8651990 - Attachment is obsolete: true
Attachment #8652032 - Flags: review+
It looks like other places are using os for b2g and android, so let's try this again w/:

> skip-if = (os != 'b2g' && os != 'android')
Attachment #8651991 - Attachment is obsolete: true
Attachment #8652035 - Flags: review?(bugmail.mozilla)
Adding r=kats to commit msg, and carrying forward r+. Also added some (much-needed) braces to nextTest.
Attachment #8651992 - Attachment is obsolete: true
Attachment #8652036 - Flags: review+
Comment on attachment 8652035 [details] [diff] [review]
976616.-Part-3-Enable-meta-viewport-tests-for-Fe.patch

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

This looks good, thanks.

::: dom/base/test/mochitest.ini
@@ +754,4 @@
>  [test_meta_viewport6.html]
> +skip-if = (os != 'b2g' && os != 'android')    # meta-viewport tag support is mobile-only
> +[test_meta_viewport7.html]
> +skip-if = buildapp != 'b2g' || toolkit != 'android'    # meta-viewport tag support is mobile-only

Technically this test_meta_viewport7.html shouldn't be in this patch, since the test is only added in part 4. Drop these two lines and rebase part 4 accordingly.
Attachment #8652035 - Flags: review?(bugmail.mozilla) → review+
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #27)
> Technically this test_meta_viewport7.html shouldn't be in this patch, since
> the test is only added in part 4. Drop these two lines and rebase part 4
> accordingly.

Yeah, that's a mistake from me trying to rebase last night -- will fix (and thanks for catching that).

Thanks for the reviews Kats and Bobby!
Adding r=kats to commit message, carrying forward r+ (and fixing the rebase mistake in this patch).
Attachment #8652035 - Attachment is obsolete: true
Attachment #8652320 - Flags: review+
Fixing the rebase mistake in this patch.
Attachment #8652036 - Attachment is obsolete: true
Attachment #8652321 - Flags: review+
OK, try looks good. Two unrelated failures on OSX, AFAICT.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/5ab594873952
https://hg.mozilla.org/mozilla-central/rev/84b869138792
https://hg.mozilla.org/mozilla-central/rev/0bc0d5200098
https://hg.mozilla.org/mozilla-central/rev/8ca999c539d7
Status: NEW → RESOLVED
Last Resolved: 4 years ago
status-firefox43: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
I added a note about this in https://developer.mozilla.org/en-US/Firefox/Releases/43#HTML
Keywords: dev-doc-needed → dev-doc-complete
Thanks Jean-Yves!
Duplicate of this bug: 976618
Depends on: 1498729
You need to log in before you can comment on or make changes to this bug.