Mike Taylor [:miketaylr] Assignee
	Description • 10 years ago

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

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 1 • 10 years ago

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.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 3 • 10 years ago

Updating title to make it easier to find.

	Mike Taylor [:miketaylr] Assignee
	Comment 4 • 10 years ago

Unsure if this should be duped against bug 617331 or bug 714737 or what. I just stumbled upon those.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 7 • 10 years ago

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

	phena109
	Comment 8 • 9 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.

	Mike Taylor [:miketaylr] Assignee
	Comment 9 • 9 years ago

I've got some local patches that have this working, will try to write some tests and post 'em in the next few days.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 10 • 9 years ago

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.

	Mike Taylor [:miketaylr] Assignee
	Comment 11 • 9 years ago

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.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 12 • 9 years ago

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.

	Mike Taylor [:miketaylr] Assignee
	Comment 13 • 9 years ago

bholley, do you mind reviewing the changes to HTMLMetaElement?

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 17 • 9 years ago

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.

	Bobby Holley (:bholley)
	Comment 18 • 9 years ago

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.

	Bobby Holley (:bholley)
	Comment 19 • 9 years ago

Oh sorry, I read HTMLMediaElement instead of HTMLMetaElement. Don't mind me!

	Mike Taylor [:miketaylr] Assignee
	Comment 20 • 9 years ago

(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.

	Mike Taylor [:miketaylr] Assignee
	Comment 21 • 9 years ago

(In reply to Bobby Holley (:bholley) from comment #19)
> Oh sorry, I read HTMLMediaElement instead of HTMLMetaElement. Don't mind me!

:)~

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 22 • 9 years ago

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 :))

	Mike Taylor [:miketaylr] Assignee
	Comment 23 • 9 years ago

Adding r=bholley to commit message and carrying forward r+.

	Mike Taylor [:miketaylr] Assignee
	Comment 24 • 9 years ago

Adding r=kats to commit msg and carrying forward r+.

	Mike Taylor [:miketaylr] Assignee
	Comment 25 • 9 years ago

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')

	Mike Taylor [:miketaylr] Assignee
	Comment 26 • 9 years ago

Adding r=kats to commit msg, and carrying forward r+. Also added some (much-needed) braces to nextTest.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 27 • 9 years ago

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.

	Mike Taylor [:miketaylr] Assignee
	Comment 28 • 9 years ago

(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!

	Mike Taylor [:miketaylr] Assignee
	Comment 29 • 9 years ago

Adding r=kats to commit message, carrying forward r+ (and fixing the rebase mistake in this patch).

	Mike Taylor [:miketaylr] Assignee
	Comment 30 • 9 years ago

Fixing the rebase mistake in this patch.

	Mike Taylor [:miketaylr] Assignee
	Comment 31 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=35d934e2bdaa

	Mike Taylor [:miketaylr] Assignee
	Comment 32 • 9 years ago

OK, try looks good. Two unrelated failures on OSX, AFAICT.

	Pulsebot
	Comment 33 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/5ab594873952
https://hg.mozilla.org/integration/mozilla-inbound/rev/84b869138792
https://hg.mozilla.org/integration/mozilla-inbound/rev/0bc0d5200098
https://hg.mozilla.org/integration/mozilla-inbound/rev/8ca999c539d7

	Ryan VanderMeulen [:RyanVM]
	Comment 34 • 9 years ago

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

	Jean-Yves Perrier [:teoli]
	Comment 35 • 9 years ago

I added a note about this in https://developer.mozilla.org/en-US/Firefox/Releases/43#HTML

	Mike Taylor [:miketaylr] Assignee
	Comment 36 • 9 years ago

Thanks Jean-Yves!