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

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Toolbar, defect)

Description

[Mike Taylor [:miketaylr]]

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

Comment 1

[Kartikaya Gupta (email:[e-mail])]

Thanks for filing this, Mike!

Copying over my comment from bug 784463:

(In reply to Kartikaya Gupta (email:[e-mail]) 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.

Comment 3

[Kartikaya Gupta (email:[e-mail])]

Updating title to make it easier to find.

Comment 4

[Mike Taylor [:miketaylr]]

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

Comment 7

[Kartikaya Gupta (email:[e-mail])]

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

Comment 8

[phena109]

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.

Comment 9

[Mike Taylor [:miketaylr]]

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

Comment 10

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 11

[Mike Taylor [:miketaylr]]

Attachment: 976616.-Support-dynamic-viewport-changes.-WIP.patch

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.

Comment 12

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 13

[Mike Taylor [:miketaylr]]

Attachment: 976616.-Part-1-Call-ProcessViewportInfo-when-met.patch

bholley, do you mind reviewing the changes to HTMLMetaElement?

Comment 14

[Mike Taylor [:miketaylr]]

Attachment: 976616.-Part-2-Update-mobile-viewport-on-DOMMeta.patch

Comment 15

[Mike Taylor [:miketaylr]]

Attachment: 976616.-Part-3-Enable-meta-viewport-tests-for-Fe.patch

Comment 16

[Mike Taylor [:miketaylr]]

Attachment: 976616.-Part-4-Add-test-for-dynamic-meta-viewpor.patch

Comment 17

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 18

[Bobby Holley (:bholley)]

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.

Comment 19

[Bobby Holley (:bholley)]

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

Comment 20

[Mike Taylor [:miketaylr]]

(In reply to Kartikaya Gupta (email:[e-mail]) 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.

Comment 21

[Mike Taylor [:miketaylr]]

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

:)~

Comment 22

[Kartikaya Gupta (email:[e-mail])]

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

Comment 23

[Mike Taylor [:miketaylr]]

Attachment: 976616.-Part-1-Call-ProcessViewportInfo-when-met.patch

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

Comment 24

[Mike Taylor [:miketaylr]]

Attachment: 976616.-Part-2-Update-mobile-viewport-on-DOMMeta.patch

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

Comment 25

[Mike Taylor [:miketaylr]]

Attachment: 976616.-Part-3-Enable-meta-viewport-tests-for-Fe.patch

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

Comment 26

[Mike Taylor [:miketaylr]]

Attachment: 976616.-Part-4-Add-test-for-dynamic-meta-viewpor.patch

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

Comment 27

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 28

[Mike Taylor [:miketaylr]]

(In reply to Kartikaya Gupta (email:[e-mail]) 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!

Comment 29

[Mike Taylor [:miketaylr]]

Attachment: 976616.-Part-3-Enable-meta-viewport-tests-for-Fe.patch

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

Comment 30

[Mike Taylor [:miketaylr]]

Attachment: 976616.-Part-4-Add-test-for-dynamic-meta-viewpor.patch

Fixing the rebase mistake in this patch.

Comment 31

[Mike Taylor [:miketaylr]]

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

Comment 32

[Mike Taylor [:miketaylr]]

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

Comment 33

[Pulsebot]

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

Comment 34

[Ryan VanderMeulen [:RyanVM]]

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

Comment 35

[Jean-Yves Perrier [:teoli]]

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

Comment 36

[Mike Taylor [:miketaylr]]

Thanks Jean-Yves!