Make chrome/ be documentElement/body agnostic with regards to scrollTop/Left

Make chrome/ be documentElement/body agnostic with regards to scrollTop/Left

Blink is in the process of getting itself in pair with how other Web engines'
(Trident,Gecko,Presto) behavior with regards to scrollTop and scrollLeft when
called for documentElement and body. This process has been proved itself
to be of great impact within Chrome, given that its UI is written in HTML/JS/CSS.
See for example bugs 305800, 304753, 305745, 304816, 305742, 305092.

This CL tries to prevent Chrome from future similar breakages, as the Blink process
of adapting to this change is not yet 100% finalized. It will also allow CLs 69143002 and 51553002  to land without breaking Chrome.

CL also fixes driven-by issues like the usage of getElementById instead of $, so CQ can land the CL.

Additionally, a PRESUBMIT hook is being considered as well, in order to
advice programmers to not user {documentElement.body}.scroll{Top,Left} directly
and instead use the helper added to resources/js/util.js


BUG=316722
R=dbeam@chromium.org,newt@chromium.org,sky@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=235414

[tonikitoo_, 2013-11-11 16:19:21]

There is a big open question in this CL: where is the best place to add the
helper.
I assume that adding that to ui/webui/resources/js/util.js is ok, and will get
visible to content in chrome/, but it might well be wrong.
I would also love to hear how can such changes be tested.

[Dan Beam, 2013-11-11 19:44:03]

https://codereview.chromium.org/68723003/diff/40001/ui/webui/resources/js/uti...
File ui/webui/resources/js/util.js (right):

https://codereview.chromium.org/68723003/diff/40001/ui/webui/resources/js/uti...
ui/webui/resources/js/util.js:327: * @param {Node} doc The document node where
information will be queried from.
all of these should be @param {!HTMLDocument}

https://codereview.chromium.org/68723003/diff/40001/ui/webui/resources/js/uti...
ui/webui/resources/js/util.js:328: * @return {value} The Y document scroll
offset.
and all of these, @return {number}

https://codereview.chromium.org/68723003/diff/40001/ui/webui/resources/js/uti...
ui/webui/resources/js/util.js:333: }
please match chrome-style not blink (2\s indent, brace on end of line).  for
example, the rest of this file's contents.

https://codereview.chromium.org/68723003/diff/40001/ui/webui/resources/js/uti...
ui/webui/resources/js/util.js:361: {
is there any reason why we shouldn't simply be checking document.compatMode and
setting the scrollLeft/scrollTop values based on the return value (e.g.
'BackCompat' or 'CSS1Compat')?

[tonikitoo_, 2013-11-11 20:27:48]

>
https://codereview.chromium.org/68723003/diff/40001/ui/webui/resources/js/uti...
> ui/webui/resources/js/util.js:327: * @param {Node} doc The document node where
> information will be queried from.
> all of these should be @param {!HTMLDocument}

Fixed.

>
https://codereview.chromium.org/68723003/diff/40001/ui/webui/resources/js/uti...
> ui/webui/resources/js/util.js:328: * @return {value} The Y document scroll
> offset.
> and all of these, @return {number}

Fixed.

>
https://codereview.chromium.org/68723003/diff/40001/ui/webui/resources/js/uti...
> ui/webui/resources/js/util.js:333: }
> please match chrome-style not blink (2\s indent, brace on end of line).  for
> example, the rest of this file's contents.

Fixed.

>
https://codereview.chromium.org/68723003/diff/40001/ui/webui/resources/js/uti...
> ui/webui/resources/js/util.js:361: {
> is there any reason why we shouldn't simply be checking document.compatMode
and
> setting the scrollLeft/scrollTop values based on the return value (e.g.
> 'BackCompat' or 'CSS1Compat')?

We could do that, but not so straight forward: we would also have to know if we
have the associated runtime flags toggled on or off ( see bug 314700).

For completeness, Dan Beam pointed me to some code doing that in
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re...
, but it was agreed to stick with the current approach.

[Dan Beam, 2013-11-11 23:46:45]

are you sure util.js is being included on all these pages?

https://chromiumcodereview.appspot.com/68723003/diff/110001/chrome/browser/re...
File chrome/browser/resources/extensions/extension_list.js (right):

https://chromiumcodereview.appspot.com/68723003/diff/110001/chrome/browser/re...
chrome/browser/resources/extensions/extension_list.js:62: var scrollTop =
$(idToHighlight).offsetTop - scrollFudge * $(idToHighlight).clientHeight;
80 col wrap

https://chromiumcodereview.appspot.com/68723003/diff/110001/ui/webui/resource...
File ui/webui/resources/js/util.js (right):

https://chromiumcodereview.appspot.com/68723003/diff/110001/ui/webui/resource...
ui/webui/resources/js/util.js:327: * @param {!HTMLDocument} doc The document
node where information will be queried from.
80 col wrap

[tonikitoo_, 2013-11-12 18:36:21]

On 2013/11/11 23:46:45, Dan Beam wrote:
> are you sure util.js is being included on all these pages?

I am not 100% sure utils.js is included in all these pages. What I did was I
tried all the bugs descriptions in the commit message (see list below) and
verified that the behavior did no regress.

bug descriptions tested: 305800, 304753, 305745, 304816, 305742, 305092

If you have a better have place to add these helps, please let me know.

[tonikitoo_, 2013-11-12 19:15:56]

On 2013/11/12 18:36:21, tonikitoo_ wrote:
> On 2013/11/11 23:46:45, Dan Beam wrote:
> > are you sure util.js is being included on all these pages?
> 
> I am not 100% sure utils.js is included in all these pages. What I did was I
> tried all the bugs descriptions in the commit message (see list below) and
> verified that the behavior did no regress.
> 
> bug descriptions tested: 305800, 304753, 305745, 304816, 305742, 305092
> 
> If you have a better have place to add these helps, please let me know.

I can also manually include like others:

$ grep -nHR "resources\/js\/util\.js" chrome | grep include
chrome/browser/resources/apps_debugger/js/main_scripts.js:17://<include
src="../../../../../ui/webui/resources/js/util.js" />
chrome/browser/resources/chromeos/wallpaper_manager/js/main_scripts.js:9://<include
src="../../../../../../ui/webui/resources/js/util.js"/>
chrome/browser/resources/file_manager/foreground/js/action_choice/action_choice_scripts.js:14://<include
src="../../../../../../../ui/webui/resources/js/util.js"/>
chrome/browser/resources/file_manager/foreground/js/main_scripts.js:31://<include
src="../../../../../../ui/webui/resources/js/util.js"/>
chrome/browser/resources/uber/uber_frame.js:10:<include
src="../../../../ui/webui/resources/js/util.js"></include>

[Dan Beam, 2013-11-12 22:53:39]

if you manually tried all the pages/bugs, that's fine.  we have no particularly
good dependency management system for webui code at this time :(, so don't add
more includes (it's likely it'll paste the contents more than once per page)

[Dan Beam, 2013-11-12 22:54:08]

please just fix up the 80 col wrap issues (i.e. that the code should wrap after
80 characters) and then it lg

[commit-bot: I haz the power, 2013-11-13 03:37:30]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer or
a lowly provisional committer, _not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[tonikitoo_, 2013-11-13 03:42:31]

On 2013/11/13 03:37:30, I haz the power (commit-bot) wrote:
> No LGTM from a valid reviewer yet. Only full committers are accepted.
> Even if an LGTM may have been provided, it was from a non-committer or
> a lowly provisional committer, _not_ a full super star committer.
> See http://www.chromium.org/getting-involved/become-a-committer
> Note that this has nothing to do with OWNERS files.

I think the CL requires "lgtm" instead of "lg".

[Dan Beam, 2013-11-13 04:39:38]

lgtm

[commit-bot: I haz the power, 2013-11-13 04:39:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/a1.gomes@sisa.samsung.com/68723003/200001

[commit-bot: I haz the power, 2013-11-13 04:55:38]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[Dan Beam, 2013-11-13 06:06:31]

https://codereview.chromium.org/68723003/diff/200001/chrome/browser/resources...
File chrome/browser/resources/about_memory.js (right):

https://codereview.chromium.org/68723003/diff/200001/chrome/browser/resources...
chrome/browser/resources/about_memory.js:6: if
(document.getElementById('helpTooltip'))
^ if util.js really is included on this page, $('helpTooltip')

https://codereview.chromium.org/68723003/diff/200001/chrome/browser/resources...
chrome/browser/resources/about_memory.js:27: var target =
document.getElementById(id);
$(id)

https://codereview.chromium.org/68723003/diff/200001/chrome/browser/resources...
chrome/browser/resources/about_memory.js:61: var el =
document.getElementById('helpTooltip');
$('helpTooltip')

https://codereview.chromium.org/68723003/diff/200001/chrome/browser/resources...
chrome/browser/resources/about_memory.js:77: var output =
document.getElementById('t');
and $('t')

[tonikitoo_, 2013-11-13 14:56:25]

On 2013/11/13 06:06:31, Dan Beam wrote:
>
https://codereview.chromium.org/68723003/diff/200001/chrome/browser/resources...
> File chrome/browser/resources/about_memory.js (right):
> 
>
https://codereview.chromium.org/68723003/diff/200001/chrome/browser/resources...
> chrome/browser/resources/about_memory.js:6: if
> (document.getElementById('helpTooltip'))
> ^ if util.js really is included on this page, $('helpTooltip')

No, it is not. Replacing getElementById by "$" fails when chrome://memory is
loaded. I managed to fix it with adding an #include.

Hopefully it is ok.

Additionally:

Files that I am able to test are (no need for explicit <include src=..>)
- chrome/browser/resources/extensions/extension_list.js
- chrome/browser/resources/uber/uber_utils.js
- chrome/browser/resources/options/options_page.js

Files that I am not able to test:
- chrome/browser/resources/help/help_base_page.js
- chrome/browser/resources/ntp_android/ntp_android.js

Any know pages I could load to test these two ^ before committing?

> chrome/browser/resources/about_memory.js:27: var target =
> document.getElementById(id);
> $(id)
> 
> chrome/browser/resources/about_memory.js:61: var el =
> document.getElementById('helpTooltip');
> $('helpTooltip')
> 
> chrome/browser/resources/about_memory.js:77: var output =
> document.getElementById('t');
> and $('t')

I can drive-by fix these, sure, but it needs me to add a <include src=...> tag,
as state above.

[tonikitoo_, 2013-11-13 15:35:11]

Update:
 
> Files that I am not able to test:
> - chrome/browser/resources/help/help_base_page.js

Ok, I managed to test help_base_page.js too, and it also works without an
include.

> - chrome/browser/resources/ntp_android/ntp_android.js

ntp_android.js is android specific, and is not presented in any .grd, so I
believe grit, and won't possibly work.

one solution could be "include" it from
chrome/browser/resources/ntp_android/new_tab.html", like it is done for other
scripts (see below)

<script src="../../../../ui/webui/resources/js/i18n_template.js"></script>

[Dan Beam, 2013-11-13 17:38:40]

On 2013/11/13 14:56:25, tonikitoo_ wrote:
> On 2013/11/13 06:06:31, Dan Beam wrote:
> >
>
https://codereview.chromium.org/68723003/diff/200001/chrome/browser/resources...
> > File chrome/browser/resources/about_memory.js (right):
> > 
> >
>
https://codereview.chromium.org/68723003/diff/200001/chrome/browser/resources...
> > chrome/browser/resources/about_memory.js:6: if
> > (document.getElementById('helpTooltip'))
> > ^ if util.js really is included on this page, $('helpTooltip')
> 
> No, it is not. Replacing getElementById by "$" fails when chrome://memory is
> loaded. I managed to fix it with adding an #include.

not lgtm

How were scrollLeftForDocument()/scrollTopForDocument() available then?

> 
> Hopefully it is ok.
> 
> Additionally:
> 
> Files that I am able to test are (no need for explicit <include src=..>)
> - chrome/browser/resources/extensions/extension_list.js
> - chrome/browser/resources/uber/uber_utils.js
> - chrome/browser/resources/options/options_page.js
> 
> Files that I am not able to test:
> - chrome/browser/resources/help/help_base_page.js
> - chrome/browser/resources/ntp_android/ntp_android.js
> 
> Any know pages I could load to test these two ^ before committing?
> 
> > chrome/browser/resources/about_memory.js:27: var target =
> > document.getElementById(id);
> > $(id)
> > 
> > chrome/browser/resources/about_memory.js:61: var el =
> > document.getElementById('helpTooltip');
> > $('helpTooltip')
> > 
> > chrome/browser/resources/about_memory.js:77: var output =
> > document.getElementById('t');
> > and $('t')
> 
> I can drive-by fix these, sure, but it needs me to add a <include src=...>
tag,
> as state above.

add an <include>

[tonikitoo_, 2013-11-13 17:43:31]

On 2013/11/13 17:38:40, Dan Beam wrote:
> On 2013/11/13 14:56:25, tonikitoo_ wrote:

> > > chrome/browser/resources/about_memory.js:6: if
> > > (document.getElementById('helpTooltip'))
> > > ^ if util.js really is included on this page, $('helpTooltip')
> > 
> > No, it is not. Replacing getElementById by "$" fails when chrome://memory is
> > loaded. I managed to fix it with adding an #include.
> 
> not lgtm
> 
> How were scrollLeftForDocument()/scrollTopForDocument() available then?

It was not working :(. It only works with the include. Sorry, I better tested it
today.

> > - chrome/browser/resources/ntp_android/ntp_android.js
> > 
> > Any know pages I could load to test these two ^ before committing?

Would love to be sure about this one still.

> > > chrome/browser/resources/about_memory.js:27: var target =
> > > document.getElementById(id);
> > > $(id)
> > > 
> > > chrome/browser/resources/about_memory.js:61: var el =
> > > document.getElementById('helpTooltip');
> > > $('helpTooltip')
> > > 
> > > chrome/browser/resources/about_memory.js:77: var output =
> > > document.getElementById('t');
> > > and $('t')
> > 
> > I can drive-by fix these, sure, but it needs me to add a <include src=...>
> tag,
> > as state above.
> 
> add an <include>

Ok.

[tonikitoo_, 2013-11-13 18:40:25]

I managed to fix about_memory.js by adding an <include src=> and editing
browser_resources.grd so its flattenhtml property is "true"

Other did not need includes:
chrome/browser/resources/extensions/extension_list.js
chrome/browser/resources/help/help_base_page.js
chrome/browser/resources/options/options_page.js
chrome/browser/resources/uber/uber_utils.js

For 
chrome/browser/resources/ntp_android/ntp_android.js  I tentatively edited the
HTML file that includes it (chrome/browser/resources/ntp_android/new_tab.html).

[Dan Beam, 2013-11-13 19:27:13]

+newt@ for chrome/browser/resources/ntp_android (could you try this patch and
verify it works?)

[newt (away), 2013-11-13 22:49:06]

verified that this works for the Android NTP.

lgtm for chrome/browser/resources/ntp_android

[Dan Beam, 2013-11-13 23:31:06]

thanks newt@! lgtm w/nits

https://codereview.chromium.org/68723003/diff/460001/chrome/browser/resources...
File chrome/browser/resources/extensions/extension_list.js (right):

https://codereview.chromium.org/68723003/diff/460001/chrome/browser/resources...
chrome/browser/resources/extensions/extension_list.js:64:
$(idToHighlight).clientHeight;
nit:  var scrollTop = $(idToHighlight).offsetTop - scrollFudge *
                      $(idToHighlight).clientHeight;

https://codereview.chromium.org/68723003/diff/460001/chrome/browser/resources...
File chrome/browser/resources/help/help_base_page.js (right):

https://codereview.chromium.org/68723003/diff/460001/chrome/browser/resources...
chrome/browser/resources/help/help_base_page.js:122: this.lastScrollTop =
scrollTop;
nit: replace `var scrollTop = ...;` with:

  this.lastScrollTop = scrollTopForDocument(document);

so it's only requested when if (freeze) is true.

https://codereview.chromium.org/68723003/diff/460001/chrome/browser/resources...
File chrome/browser/resources/options/options_page.js (right):

https://codereview.chromium.org/68723003/diff/460001/chrome/browser/resources...
chrome/browser/resources/options/options_page.js:208: window.scroll(scrollLeft,
scrollTop);
nit: window.scroll(scrollLeftForDocument(document), scrollTop);

[commit-bot: I haz the power, 2013-11-14 03:47:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/a1.gomes@sisa.samsung.com/68723003/600001

[commit-bot: I haz the power, 2013-11-14 04:04:15]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[tonikitoo_, 2013-11-14 05:48:58]

adding @sky for the grd bits.

[sky, 2013-11-14 15:37:13]

LGTM

[commit-bot: I haz the power, 2013-11-14 15:42:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/a1.gomes@sisa.samsung.com/68723003/600001

[commit-bot: I haz the power, 2013-11-14 19:09:19]

Retried try job too often on win_rel for step(s) nacl_integration
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-11-14 19:21:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/a1.gomes@sisa.samsung.com/68723003/600001

[commit-bot: I haz the power, 2013-11-14 22:16:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/a1.gomes@sisa.samsung.com/68723003/600001

[commit-bot: I haz the power, 2013-11-15 02:05:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/a1.gomes@sisa.samsung.com/68723003/600001

[commit-bot: I haz the power, 2013-11-15 04:09:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/a1.gomes@sisa.samsung.com/68723003/600001

[commit-bot: I haz the power, 2013-11-15 08:24:17]

Retried try job too often on win_rel for step(s) nacl_integration
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-11-15 14:37:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/a1.gomes@sisa.samsung.com/68723003/600001

[commit-bot: I haz the power, 2013-11-15 19:11:25]

Retried try job too often on win_rel for step(s) nacl_integration
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-11-15 19:24:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/a1.gomes@sisa.samsung.com/68723003/600001

[commit-bot: I haz the power, 2013-11-15 20:34:35]

Change committed as 235414