Closed Bug 1083932 Opened 6 years ago Closed 3 years ago

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

Categories

(Web Compatibility :: Mobile, defect)

All
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED WORKSFORME

People

(Reporter: gcp, Assigned: karlcow)

References

(Blocks 1 open bug, )

Details

(Whiteboard: [contactready][js][country-all])

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.
Probably evang, Mike do you have some time to check?
Flags: needinfo?(miket)
OS: Linux → Android
Hardware: x86_64 → All
Yep, I see the problem as well. Will investigate.
Assignee: nobody → miket
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
Component: General → Mobile
Product: Firefox for Android → Tech Evangelism
Whiteboard: [notcontactready]
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.
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?
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.
Summary: Google News dropdown menus do not work → Google News dropdown menus rely on setting non-standard document.body.scrollTop
Whiteboard: [notcontactready] → [contactready][js][country-all]
Assignee: miket → nobody
Mike, 
Does that mean which should hold on this?
We do the correct thing if I understood.
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.
(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.
Now is good, it will give them a heads up to fix their site for their own users. :)
Contacted Google today.
Assignee: nobody → kdubost
Status: NEW → ASSIGNED
(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.
Flags: needinfo?(mihai.sucan)
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.
(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
Still the same issue with Firefox Android + version number.
Duplicate of this bug: 1195316
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)
Flags: needinfo?(miket)
> 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.
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.
Flags: needinfo?(miket)
The site's been redesigned it seems. -> WFM
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → WORKSFORME
Product: Tech Evangelism → Web Compatibility
Flags: needinfo?(Rabbiulahmed00)
Restrict Comments: true
You need to log in before you can comment on or make changes to this bug.