1083932 - Google News dropdown menus rely on setting non-standard document.body.scrollTop

Status: RESOLVED WORKSFORME

(Web Compatibility :: Mobile, defect)

Description

[Gian-Carlo Pascutto [:gcp]]

1) Navigate to news.google.com
2) Click the dropdown menu icon on the top left
3) Click a section
4) Observe no result
5) Click the "different version..." link on the bottom
6) Observe it working

This works in Chrome. Not sure if it's evangelism or a Firefox bug.

Comment 1

[Kevin Brosnan [Ex-Mozilla]]

Probably evang, Mike do you have some time to check?

Comment 2

[Mike Taylor [:miketaylr]]

Yep, I see the problem as well. Will investigate.

Comment 3

[Mike Taylor [:miketaylr]]

Need to shift gears for a while, here's what I've learned so far:

Navigation to the chosen section appears to happen here,

sp[w].Zb = function(a) {
  sp.A.Zb[B](this, a);
  if (a = a[md].gb("navId")) {
    var b = new pp;
    b.xe(a);
    b.R()
  }
};

"navId" is a custom attribute on the menu item,
 
<div id=":3c" class="Nd Ad" navid="en_us:b" type="b">
  <div class="Nd-jb"></div><div class="Ad-d a O-d-y"></div><div class="Ad-Rc">Business</div>
</div>

b is an instance of pp, and R is defined as,

pp[w].R = function() {
  try {
    var a = ie("NewsContentPane"), b = this.kc(), c = a.hc, d = c.me(b);
    c.Wo(d);
    pp.A.R[B](this)
  } catch (e) {
    NEWS_logError(e, "While trying to navigate to section in T1.")
  }
};

I need to look more closely at C.Wo, which seems to be where the actual scrolling is happening (but maybe elsewhere...): https://gist.github.com/miketaylr/27944a1c205d96b6b8dd

Comment 4

[Mike Taylor [:miketaylr]]

Note, this reproduces in Desktop if you spoof UA, but does not reproduce in Chrome if you spoof as Firefox Mobile. So it's likely not a UA sniffing bug.

Comment 5

[Mike Taylor [:miketaylr]]

Digging through stack traces some more, I'm confused at the following. In ks[w].kf, eventually you get to:

 this[Xd].scrollTop = k[cd](this[Od][1])

Which sets scrollTop on document.body. With a breakpoint set on that line, k is the Math builtin and cd is the string "round" in Chrome.

Looking at this in our devtools, while remote debugging, k is `Math`, but k[cd] is undefined. `cd` is function JSTH_cd()... which is defined in the devtools source: http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/webconsole/utils.js#1649.

 I guess the global command is overriding the local scope variable. I doubt that's the bug at hand, just makes it tricky to figure out what the bug is...

Mihai, is there any way to disable these kinds of console commands during step debugging?

Comment 6

[Mike Taylor [:miketaylr]]

Oh, OK. So the previous `cd` issue is unrelated (but weird). It's the old setting scrollTop on document.body vs document.documentElement in standards mode vs Quirks Mode. 

It looks like Chrome is trying to get this fixed (behind a flag). cf. https://code.google.com/p/chromium/issues/detail?id=157855 and https://code.google.com/p/chromium/issues/detail?id=382889

I'll leave a comment in those bugs--the "correct" fix would be to set scrollTop of the documentElement. But since that would break Chrome, they need to account for their quirky implementation until they ship the fix.

Comment 7

[Karl Dubost💡 :karlcow]

Mike, 
Does that mean which should hold on this?
We do the correct thing if I understood.

Comment 8

[Mike Taylor [:miketaylr]]

Karl, not sure I understand entirely your question--but we should contact Google and ask them to account for the spec-described behavior. It seems like they want to move that way, so it's in their interest as well.

Comment 9

[Karl Dubost💡 :karlcow]

(In reply to Mike Taylor [:miketaylr] from comment #8)
> Karl, not sure I understand entirely your question--but we should contact
> Google and ask them to account for the spec-described behavior. It seems
> like they want to move that way, so it's in their interest as well.

Hehe, sorry. I meant if we contacting them now or later when they have fixed Chrome.

Comment 10

[Mike Taylor [:miketaylr]]

Now is good, it will give them a heads up to fix their site for their own users. :)

Comment 11

[Karl Dubost💡 :karlcow]

Contacted Google today.

Comment 12

[Mihai Sucan [:msucan]]

(In reply to Mike Taylor [:miketaylr] from comment #5)
> Mihai, is there any way to disable these kinds of console commands during
> step debugging?

Unfortunately no. Please file a bug about this problem.

Comment 13

[PhistucK]

For your information, since the Chrome Developer Tools feature uses -
with (_commandLineAPI || {}) { code_to_evaluate }
Or something similar, to evaluate code, you can evaluate -
} cd; if (1) {
And it will disregard the console API. Perhaps this is also true in the Firefox Developer Tools as a quick hack until the bug is fixed.

Comment 14

[Hallvord R. M. Steen [:hallvors]]

(In reply to Mihai Sucan [:msucan] from comment #12)
> Unfortunately no. Please file a bug about this problem.
Perhaps grabbing the limelight from Mike, I've taken the liberty of reporting https://bugzilla.mozilla.org/show_bug.cgi?id=1113954

Comment 15

[Karl Dubost💡 :karlcow]

Still the same issue with Firefox Android + version number.

Comment 17

[Karl Dubost💡 :karlcow]

Mike,

(In reply to Mike Taylor [:miketaylr] from comment #6)
> Oh, OK. So the previous `cd` issue is unrelated (but weird). It's the old
> setting scrollTop on document.body vs document.documentElement in standards
> mode vs Quirks Mode. 
> 
> It looks like Chrome is trying to get this fixed (behind a flag). cf.
> https://code.google.com/p/chromium/issues/detail?id=157855 and
> https://code.google.com/p/chromium/issues/detail?id=382889
> 
> I'll leave a comment in those bugs--the "correct" fix would be to set
> scrollTop of the documentElement. But since that would break Chrome, they
> need to account for their quirky implementation until they ship the fix.


https://bugs.chromium.org/p/chromium/issues/detail?id=157855 has been pushed down in terms of priority because of https://bugs.chromium.org/p/chromium/issues/detail?id=501568

The other bug is fixed
https://bugs.chromium.org/p/chromium/issues/detail?id=382889

I tested today again. And the issue is still happening on the menu.

Should we recommend Google to fix with 
(document.documentElement.scrollTop||document.body.scrollTop)

Comment 18

[PhistucK]

> Should we recommend Google to fix with (document.documentElement.scrollTop||document.body.scrollTop)

The latest advice is to try using document.scrollingElement first and fallback to either what you suggested (preferred) or whatever it is that is currently being used.

According to https://developer.mozilla.org/en/docs/Web/API/document/scrollingElement - document.scrollingElement will be supported in Firefox 47.

Comment 19

[Mike Taylor [:miketaylr]]

Karl,

I agree with Comment #18 -- the safest thing for them to do is use scrollingElement (which they proposed to solve this exact problem). But since we won't support that until 47 is in Release, it would remain just as broken for us.

Some combination of document.scrollingElement (if it exists) and UA sniffing (for WebKit) to choose between document.documentElement and document.body as a fallback is probably the most cross-browser solution, as crappy as that sounds.

Comment 20

[Mike Taylor [:miketaylr]]

The site's been redesigned it seems. -> WFM

Comment 21

[Rabbiul Ahmed]

I'm New Blogger https://tazabollywoodnews.com

Comment 22

[Rabbiul Ahmed]

<a href="https://tazabollywoodnews.com">Tazabollywoodnews</a>