Refresh the Incognito NTP layout on default font size change

Refresh the Incognito NTP layout on default font size change

The maximum width of the Incognito NTP content is not described by CSS,
but computed by JS as a function of the font size. Therefore, it must
be recomputed on font size changes.

BUG=693525
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2905433002
Cr-Commit-Position: refs/heads/master@{#474546}
Committed: https://chromium.googlesource.com/chromium/src/+/81064bed1ae35ec03307b6d7ffee8336386429b1

[msramek, 2017-05-23 19:55:50]

Description was changed from

==========
Refresh the Incognito NTP layout on default font size change

The maximum width of the Incognito NTP content is not described by CSS,
but computed by JS as a function of the font size. Therefore, it must
be recomputed on font size changes.

BUG=693525
==========

to

==========
Refresh the Incognito NTP layout on default font size change

The maximum width of the Incognito NTP content is not described by CSS,
but computed by JS as a function of the font size. Therefore, it must
be recomputed on font size changes.

BUG=693525
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[msramek, 2017-05-23 19:57:21]

Patchset #1 (id:1) has been deleted

[msramek, 2017-05-23 19:57:29]

The CQ bit was checked by msramek@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-23 19:58:15]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[msramek, 2017-05-23 19:59:18]

msramek@chromium.org changed reviewers:
+ dbeam@chromium.org

[msramek, 2017-05-23 19:59:19]

Hi Dan,

Sorry for spamming you lately, but please have a look :)

This CL solves a corner case which is unlikely to happen in real-life usage, but
actually happens a lot during testing, so I wanted to fix it. But if you think
the purpose does not justify the complexity, feel free to throw it out :)

Thanks!
Martin

[commit-bot: I haz the power, 2017-05-23 21:34:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-23 21:34:33]

Dry run: This issue passed the CQ dry run.

[Dan Beam, 2017-05-24 03:09:19]

what about zoom changes?

i know it's nice to be webby, but is there a way you could just observe the zoom
in the browser and send notifications?

https://codereview.chromium.org/2905433002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/ntp4/md_incognito_tab.js (right):

https://codereview.chromium.org/2905433002/diff/20001/chrome/browser/resource...
chrome/browser/resources/ntp4/md_incognito_tab.js:47:
bulletpoints[1].classList.remove('tooWide');
nit:

var MAX_ALLOWED_WIDTH = 600;

var tooWide = maxWidth > MAX_ALLOWED_WIDTH;
bulletpoints[1].classList.add('tooWide', tooWide);
if (tooWide)
  maxWidth = MAX_ALLOWED_WIDTH;

https://codereview.chromium.org/2905433002/diff/20001/chrome/browser/resource...
chrome/browser/resources/ntp4/md_incognito_tab.js:51: };
no semi

https://codereview.chromium.org/2905433002/diff/20001/chrome/browser/resource...
chrome/browser/resources/ntp4/md_incognito_tab.js:57: if (ResizeObserver) {
errr, why do you need to feature detect this?

https://codereview.chromium.org/2905433002/diff/20001/chrome/browser/resource...
chrome/browser/resources/ntp4/md_incognito_tab.js:59:
observer.observe(fontSizeEstimator);
new ResizeObserver(recomputeLayoutWidth).observe(fontSizeEstimator);

https://codereview.chromium.org/2905433002/diff/20001/chrome/browser/resource...
chrome/browser/resources/ntp4/md_incognito_tab.js:64: if
(window.getComputedStyle(document.body).fontSize == currentFontSize)
why are you calling this so much?  this is a mildly expensive call...

[msramek, 2017-05-24 12:55:11]

On 2017/05/24 03:09:19, Dan Beam wrote:
> what about zoom changes?

Zooming doesn't cause this problem. I measured the offsetWidth for both .content
and .bulletpoints, and it's invariant w.r.t. zoom level.

> 
> i know it's nice to be webby, but is there a way you could just observe the
zoom
> in the browser and send notifications?
> 

Okay, I changed the code to observe the default font size pref. However, there's
a problem - the pref observer can send a JS message to the NTP and run the
recalculation script *faster* than Blink manages to react to the change and
reflow the layout. Which means that at the time we re-measure the offsetWidths,
they still have their old values.

I tried spinning the message loop by setting setTimeout(,0), but it's not
enough. So I added a magic constant of 100 milliseconds. It's not perfect, but I
guess it's better than my previous solution of polling every second.

[msramek, 2017-05-24 12:56:02]

Thanks, Dan. So I tried a different approach :)

https://codereview.chromium.org/2905433002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/ntp4/md_incognito_tab.js (right):

https://codereview.chromium.org/2905433002/diff/20001/chrome/browser/resource...
chrome/browser/resources/ntp4/md_incognito_tab.js:47:
bulletpoints[1].classList.remove('tooWide');
On 2017/05/24 03:09:19, Dan Beam wrote:
> nit:
> 
> var MAX_ALLOWED_WIDTH = 600;
> 
> var tooWide = maxWidth > MAX_ALLOWED_WIDTH;
> bulletpoints[1].classList.add('tooWide', tooWide);
> if (tooWide)
>   maxWidth = MAX_ALLOWED_WIDTH;

bulletpoints[1].classList.add('tooWide', tooWide) would add two classes -
"tooWide" and "true"/"false". I think you're confusing this with setAttribute()?

That said, we can change this to an attribute as well.

https://codereview.chromium.org/2905433002/diff/20001/chrome/browser/resource...
chrome/browser/resources/ntp4/md_incognito_tab.js:51: };
On 2017/05/24 03:09:19, Dan Beam wrote:
> no semi

Done.

https://codereview.chromium.org/2905433002/diff/20001/chrome/browser/resource...
chrome/browser/resources/ntp4/md_incognito_tab.js:57: if (ResizeObserver) {
On 2017/05/24 03:09:19, Dan Beam wrote:
> errr, why do you need to feature detect this?

It has existed for a few milestones, but it's still behind the experimental web
features flag. Anyways, I removed it for now.

https://codereview.chromium.org/2905433002/diff/20001/chrome/browser/resource...
chrome/browser/resources/ntp4/md_incognito_tab.js:59:
observer.observe(fontSizeEstimator);
On 2017/05/24 03:09:19, Dan Beam wrote:
> new ResizeObserver(recomputeLayoutWidth).observe(fontSizeEstimator);

Acknowledged, but I removed this code.

https://codereview.chromium.org/2905433002/diff/20001/chrome/browser/resource...
chrome/browser/resources/ntp4/md_incognito_tab.js:64: if
(window.getComputedStyle(document.body).fontSize == currentFontSize)
On 2017/05/24 03:09:19, Dan Beam wrote:
> why are you calling this so much?  this is a mildly expensive call...

Acknowledged, but I removed this code.

[msramek, 2017-05-24 12:56:53]

The CQ bit was checked by msramek@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-24 12:57:12]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-24 14:12:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-24 14:12:48]

Dry run: This issue passed the CQ dry run.

[Dan Beam, 2017-05-24 17:43:54]

https://codereview.chromium.org/2905433002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/ntp4/md_incognito_tab.js (right):

https://codereview.chromium.org/2905433002/diff/20001/chrome/browser/resource...
chrome/browser/resources/ntp4/md_incognito_tab.js:47:
bulletpoints[1].classList.remove('tooWide');
On 2017/05/24 12:56:02, msramek wrote:
> On 2017/05/24 03:09:19, Dan Beam wrote:
> > nit:
> > 
> > var MAX_ALLOWED_WIDTH = 600;
> > 
> > var tooWide = maxWidth > MAX_ALLOWED_WIDTH;
> > bulletpoints[1].classList.add('tooWide', tooWide);
> > if (tooWide)
> >   maxWidth = MAX_ALLOWED_WIDTH;
> 
> bulletpoints[1].classList.add('tooWide', tooWide) would add two classes -
> "tooWide" and "true"/"false". I think you're confusing this with
setAttribute()?
> 
> That said, we can change this to an attribute as well.

sorry, classList.toggle

[msramek, 2017-05-24 23:08:35]

The CQ bit was checked by msramek@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-24 23:09:21]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[msramek, 2017-05-24 23:09:39]

Patchset #3 (id:60001) has been deleted

[msramek, 2017-05-24 23:10:00]

The CQ bit was checked by msramek@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-24 23:10:05]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[msramek, 2017-05-24 23:16:37]

https://codereview.chromium.org/2905433002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/ntp4/md_incognito_tab.js (right):

https://codereview.chromium.org/2905433002/diff/20001/chrome/browser/resource...
chrome/browser/resources/ntp4/md_incognito_tab.js:47:
bulletpoints[1].classList.remove('tooWide');
On 2017/05/24 17:43:54, Dan Beam wrote:
> On 2017/05/24 12:56:02, msramek wrote:
> > On 2017/05/24 03:09:19, Dan Beam wrote:
> > > nit:
> > > 
> > > var MAX_ALLOWED_WIDTH = 600;
> > > 
> > > var tooWide = maxWidth > MAX_ALLOWED_WIDTH;
> > > bulletpoints[1].classList.add('tooWide', tooWide);
> > > if (tooWide)
> > >   maxWidth = MAX_ALLOWED_WIDTH;
> > 
> > bulletpoints[1].classList.add('tooWide', tooWide) would add two classes -
> > "tooWide" and "true"/"false". I think you're confusing this with
> setAttribute()?
> > 
> > That said, we can change this to an attribute as well.
> 
> sorry, classList.toggle

Ah, right. Done.

[Dan Beam, 2017-05-25 01:02:39]

lgtm w/nits

https://codereview.chromium.org/2905433002/diff/80001/chrome/browser/resource...
File chrome/browser/resources/ntp4/md_incognito_tab.js (right):

https://codereview.chromium.org/2905433002/diff/80001/chrome/browser/resource...
chrome/browser/resources/ntp4/md_incognito_tab.js:28:
bulletpoints[1].classList.toggle('tooWide', tooWide);
classes should be dash-form

https://codereview.chromium.org/2905433002/diff/80001/chrome/browser/resource...
chrome/browser/resources/ntp4/md_incognito_tab.js:54:
setTimeout(recomputeLayoutWidth, 100);
why are you doing this on a 100ms delay?  can we use requestAnimationFrame()
instead?

[commit-bot: I haz the power, 2017-05-25 01:05:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-25 01:05:19]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[msramek, 2017-05-25 01:49:36]

Thanks!

Unfortunately I'll have to leave the 100ms constant there, because I don't think
there's a way around it, at least on JS side. It's not elegant, but it's good
enough and sufficiently lightweight solution to the bug I'm fixing.

https://codereview.chromium.org/2905433002/diff/80001/chrome/browser/resource...
File chrome/browser/resources/ntp4/md_incognito_tab.js (right):

https://codereview.chromium.org/2905433002/diff/80001/chrome/browser/resource...
chrome/browser/resources/ntp4/md_incognito_tab.js:28:
bulletpoints[1].classList.toggle('tooWide', tooWide);
On 2017/05/25 01:02:39, Dan Beam wrote:
> classes should be dash-form

Done.

https://codereview.chromium.org/2905433002/diff/80001/chrome/browser/resource...
chrome/browser/resources/ntp4/md_incognito_tab.js:54:
setTimeout(recomputeLayoutWidth, 100);
On 2017/05/25 01:02:39, Dan Beam wrote:
> why are you doing this on a 100ms delay?  can we use requestAnimationFrame()
> instead?

No, that's still too soon.

There is simply a delay somewhere between changing the font size in settings and
Blink redrawing the page. I'm not sure where it comes from, but I see it on
other pages, even chrome://settings itself. We manage to run JS code (this
method) before the font size is actually updated, so the delay is somewhere in
the C++ code, and not represented by a JS event.

I just chose 100ms because it seems to be large enough for the font size to
update, but small enough so that it doesn't look bad...

[msramek, 2017-05-25 01:57:47]

Patchset #4 (id:100001) has been deleted

[msramek, 2017-05-25 01:59:54]

The CQ bit was checked by msramek@chromium.org

[msramek, 2017-05-25 01:59:54]

The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org
Link to the patchset: https://codereview.chromium.org/2905433002/#ps120001
(title: "s/tooWide/too-wide/")

[commit-bot: I haz the power, 2017-05-25 02:00:48]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Dan Beam, 2017-05-25 03:11:27]

https://codereview.chromium.org/2905433002/diff/80001/chrome/browser/resource...
File chrome/browser/resources/ntp4/md_incognito_tab.js (right):

https://codereview.chromium.org/2905433002/diff/80001/chrome/browser/resource...
chrome/browser/resources/ntp4/md_incognito_tab.js:54:
setTimeout(recomputeLayoutWidth, 100);
On 2017/05/25 01:49:35, msramek wrote:
> On 2017/05/25 01:02:39, Dan Beam wrote:
> > why are you doing this on a 100ms delay?  can we use requestAnimationFrame()
> > instead?
> 
> No, that's still too soon.
> 
> There is simply a delay somewhere between changing the font size in settings
and
> Blink redrawing the page. I'm not sure where it comes from, but I see it on
> other pages, even chrome://settings itself. We manage to run JS code (this
> method) before the font size is actually updated, so the delay is somewhere in
> the C++ code, and not represented by a JS event.
> 
> I just chose 100ms because it seems to be large enough for the font size to
> update, but small enough so that it doesn't look bad...

for what it's worth, there's a very very low chance this works for everybody. 
if you really need this to work 100%, you should poll until your condition is
satisfied.

[commit-bot: I haz the power, 2017-05-25 04:12:39]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1495677594278910,
"parent_rev": "4e58cf4b5a9ff2a3e20e65d984a03f6670cce5e9", "commit_rev":
"81064bed1ae35ec03307b6d7ffee8336386429b1"}

[commit-bot: I haz the power, 2017-05-25 04:12:52]

Description was changed from

==========
Refresh the Incognito NTP layout on default font size change

The maximum width of the Incognito NTP content is not described by CSS,
but computed by JS as a function of the font size. Therefore, it must
be recomputed on font size changes.

BUG=693525
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Refresh the Incognito NTP layout on default font size change

The maximum width of the Incognito NTP content is not described by CSS,
but computed by JS as a function of the font size. Therefore, it must
be recomputed on font size changes.

BUG=693525
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2905433002
Cr-Commit-Position: refs/heads/master@{#474546}
Committed:
https://chromium.googlesource.com/chromium/src/+/81064bed1ae35ec03307b6d7ffee...
==========

[commit-bot: I haz the power, 2017-05-25 04:12:53]

Committed patchset #4 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/81064bed1ae35ec03307b6d7ffee...