Dynamic page scaling in amp-story #12901

alanorozco · 2018-01-18T22:34:50Z

Scales pages so that they are sized within a pixel range, but rendered at the correct visual dimensions.

Guarded by amp-story-scaling experiment flag.

Can be turned on at the page level: <amp-story-page scaling="relative">.

Implementation

~~Uses a sentinel sizer element.~~ Uses active page for sizing.
Keeps target dimensions at the story level.
Updates dimensions on viewport resize.
Applies scaling to direct amp-story-grid-layer descendants.
Backed by zoom or scale.
Uses CSS vars when possible so layers don't need to be updated on resize.
Lazily applied. Will not update dimensions for elements until they come in view.

Pending

Validation rules for opt-out. (Coming in separate PR).
~~Use scale on Firefox, as it has no support for zoom.~~ Done.
~~Calculate relative layer size (currently assumes full container size).~~ Done.

alanorozco · 2018-01-18T22:52:21Z

extensions/amp-story/0.1/page-scaling.js

+ * @return {boolean}
+ */
+function isScalingEnabled(page) {
+  return page.getAttribute('scaling') != 'disabled';


Not super convinced about this API 🤷

I think this is okay, at least for now. Thinking about it more, we definitely want to try it as opt-out behind an experiment first, so that we don't need publishers to republish stories in order to evaluate the feature.

That said, maybe we should brainstorm the explicit values we expect? This is logically kind of similar to object-fit, so maybe we can steal their API?

No scaling can be scaling="none"

This implementation is like scaling="contain" (or, at least for now, omitting the attribute altogether)

We can add other modes later if we choose to

Maybe we should name it accordingly? content-fit?

Not really sure about any of the details here, but I think we should check that the attribute is some concrete value instead of checking that it's not disabled.

Actually, maybe we can do both opt-in and opt-out? Assuming the attribute names from above (though I'm not particularly set on those names), we can do:

Experiment OFF Experiment ON

(no attribute) scaling disabled scaling enabled

scaling="none" scaling disabled scaling disabled

scaling="contain" scaling enabled scaling enabled

In other words, explicitly setting a value is always respected, but the default (no attribute) case is controlled by the experiment. This will allow us to meet two goals:

Test the feature on ALL existing stories, without having to change their markup

Allow publishers to publish new stories making use of the feature, by explicitly opting in on a story-by-story basis

WDYT? Is this feasible? Can we move the experiment check to isScalingEnabled?

newmuis

For your pending items:

I think validation can come in a separate PR? As long as the experiment is in place, the opt out API doesn't need to be valid.
SGTM to use scale where zoom is unsupported. Can we actually check feature support instead of checking user agent? Or does Firefox claim they support zoom but actually not support our use case?
I think we will soon-ish be adding layers that are not full container size. All layers are currently full container size, though. I'll leave it up to you whether you want to tackle that in this PR or separately.

newmuis · 2018-01-19T13:15:58Z

extensions/amp-story/0.1/amp-story-desktop.css

  max-height: 75vh !important;
  max-width: calc(3/5 * 75vh) !important;
  min-width: 320px !important;
  min-height: 533px !important;
 }

+[desktop] > amp-story-page,
+[desktop] .i-amphtml-story-page-sentinel {
+  left: 50%!important;


nit: space before !important

Reverted CSS changes.

newmuis · 2018-01-19T13:19:27Z

extensions/amp-story/0.1/page-scaling.js

+ * @return {boolean}
+ */
+function isScalingEnabled(page) {
+  return page.getAttribute('scaling') != 'disabled';


I think this is okay, at least for now. Thinking about it more, we definitely want to try it as opt-out behind an experiment first, so that we don't need publishers to republish stories in order to evaluate the feature.

That said, maybe we should brainstorm the explicit values we expect? This is logically kind of similar to object-fit, so maybe we can steal their API?

No scaling can be scaling="none"

This implementation is like scaling="contain" (or, at least for now, omitting the attribute altogether)

We can add other modes later if we choose to

Maybe we should name it accordingly? content-fit?

Not really sure about any of the details here, but I think we should check that the attribute is some concrete value instead of checking that it's not disabled.

newmuis · 2018-01-19T13:27:39Z

extensions/amp-story/0.1/page-scaling.js

+
+
+/** @private @const {number} */
+const MIN_LAYER_WIDTH_PX = 380;


How did we pick these min/max values? Is there a way to keep these in sync with the actual min/max widths that we allow in our UIs? e.g. I think the mobile UI can go larger than 520 before it hits the desktop breakpoint, and I think it can go smaller than 380 as well.

Ignore this; I got it backwards. I thought we were scaling within the range min...max, but we're scaling outside the range min...max.

newmuis · 2018-01-19T13:28:39Z

extensions/amp-story/0.1/page-scaling.js

+  const style = {
+    'width': px(width),
+    'height': px(height),
+    'zoom': `${factor * 100}%`,


FWIW, I think you can use decimal zooms (e.g. 0.9) instead of percentage (e.g. 90%), so you may just be able to use factor directly.

alanorozco · 2018-01-26T17:08:43Z

@newmuis PTAL

I'm not exactly sure about the scaling enabled API. I used outside-range since it seems accurate, but I also find it a little bit confusing.

newmuis · 2018-01-26T17:20:45Z

extensions/amp-story/0.1/page-scaling.js

+  updateRootProps() {
+    const {width, height, factor} = this.targetDimensions_;
+    this.vsync_.mutate(() => {
+      this.rootEl_.style.setProperty('--i-amphtml-story-width', px(width));


Is the goal that this is user-overridable? I think the fact that it's --i-amphtml-* will make the doc invalid if people try to override this.

It's not user-overridable. The purpose of using CSS vars is to prevent the layers from being updated on resize, since they reference a css var that is defined at the story element level

newmuis · 2018-01-30T01:11:30Z

Thanks @alanorozco!

googlebot added the cla: yes label Jan 18, 2018

Implement amp-story dynamic page scaling

26488aa

alanorozco force-pushed the scaling branch 3 times, most recently from 272bdb6 to c52e40b Compare January 18, 2018 22:50

alanorozco changed the title ~~Work in progress: Implement amp-story dynamic page scaling~~ Dynamic page scaling in amp-story Jan 18, 2018

alanorozco requested a review from newmuis January 18, 2018 22:51

alanorozco commented Jan 18, 2018

View reviewed changes

Register experiment

6003550

alanorozco force-pushed the scaling branch from c52e40b to 6003550 Compare January 19, 2018 03:04

newmuis reviewed Jan 19, 2018

View reviewed changes

alanorozco added 2 commits January 19, 2018 09:56

Merge branch 'master' of github.com:ampproject/amphtml into scaling

d120d34

Merge branch 'master' of github.com:ampproject/amphtml into scaling

2f67f7d

alanorozco force-pushed the scaling branch from b9bb2bf to 2f67f7d Compare January 19, 2018 22:15

alanorozco added 4 commits January 26, 2018 08:57

Fallback transform implementation + custom css props

8c05587

Merge branch 'master' of github.com:ampproject/amphtml into scaling

e575be8

Revert CSS changes

0645c6b

Oops

3e9d4cf

newmuis approved these changes Jan 26, 2018

View reviewed changes

newmuis reviewed Jan 26, 2018

View reviewed changes

alanorozco and others added 8 commits January 26, 2018 10:15

Fallback to scale on iOs

ab6b31b

Rewrite experiment description for clarity

fb6e748

Add TODO

82b6137

Whoooooops

6c02375

Style

6cd35df

Don't assert until scale time

ccdf4a7

Update amp-story.js

9c509b0

Update amp-story.js

6a6c478

alanorozco merged commit e4d34fc into ampproject:master Jan 30, 2018

alanorozco deleted the scaling branch January 30, 2018 00:27

alanorozco mentioned this pull request Jan 31, 2018

Automatically scale content to fit responsively #12548

Closed

gzgogo pushed a commit to gzgogo/amphtml that referenced this pull request Feb 14, 2018

Dynamic page scaling in amp-story (ampproject#12901)

659288d

gzgogo pushed a commit to gzgogo/amphtml that referenced this pull request Feb 14, 2018

Dynamic page scaling in amp-story (ampproject#12901)

e4cdfc7

gzgogo pushed a commit to gzgogo/amphtml that referenced this pull request Feb 14, 2018

Dynamic page scaling in amp-story (ampproject#12901)

274d3d2

RanAbram pushed a commit to RanAbram/amphtml that referenced this pull request Mar 12, 2018

Dynamic page scaling in amp-story (ampproject#12901)

1c2b99e

protonate pushed a commit to protonate/amphtml that referenced this pull request Mar 15, 2018

Dynamic page scaling in amp-story (ampproject#12901)

a269220

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dynamic page scaling in amp-story #12901

Dynamic page scaling in amp-story #12901

alanorozco commented Jan 18, 2018 •

edited

alanorozco Jan 18, 2018

newmuis Jan 19, 2018

newmuis Jan 19, 2018

newmuis left a comment

newmuis Jan 19, 2018

alanorozco Jan 26, 2018

newmuis Jan 19, 2018

newmuis Jan 19, 2018

newmuis Jan 19, 2018

newmuis Jan 19, 2018

alanorozco commented Jan 26, 2018

newmuis Jan 26, 2018

alanorozco Jan 26, 2018 •

edited

newmuis commented Jan 30, 2018

	Experiment OFF	Experiment ON
(no attribute)	scaling disabled	scaling enabled
`scaling="none"`	scaling disabled	scaling disabled
`scaling="contain"`	scaling enabled	scaling enabled



		/** @private @const {number} */
		const MIN_LAYER_WIDTH_PX = 380;

Dynamic page scaling in amp-story #12901

Dynamic page scaling in amp-story #12901

Conversation

alanorozco commented Jan 18, 2018 • edited

Implementation

Pending

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

newmuis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alanorozco commented Jan 26, 2018

Choose a reason for hiding this comment

alanorozco Jan 26, 2018 • edited

Choose a reason for hiding this comment

newmuis commented Jan 30, 2018

alanorozco commented Jan 18, 2018 •

edited

alanorozco Jan 26, 2018 •

edited