392671 - google reader jumpy when tagging

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[cannedbrain+mozilla]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en; rv:1.9a8pre) Gecko/2007081701 Camino/2.0a1pre (like Firefox/3.0a8pre)
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en; rv:1.9a8pre) Gecko/2007081701 Camino/2.0a1pre (like Firefox/3.0a8pre)

When adding a tag to an article in google reader, the page would jump one-two pages down and auto complete does not work. It doesn't happen in 1.6pre1.

Reproducible: Always

Steps to Reproduce:
1. click "Add tags"
2. start typing
Actual Results:  
The page would automagically scroll down and it won't autocomplete as you start typing.

Expected Results:  
Should stay where it it & display matching tags as you type.

Here's the errors found in Console:
https://dav.earthlink.net/cannedbrain/Public/jserrors.txt

Comment 1

[cannedbrain+mozilla]

Btw, here are the corresponding files/urls in the error log:
http://www.google.com/reader/ui/1374423557-settings.css
http://www.google.com/reader/ui/2796695252-scroll.css
http://www.google.com/reader/view/feed/http%3A%2F%2Fnewsrss.bbc.co.uk%2Frss%2Fnewsonline_world_edition%2Fasia-pacific%2Frss.xml
https://www.google.com/reader/ui/2796695252-scroll.css
https://www.google.com/reader/view/

Comment 2

[Chris Lawson (gone)]

The fact that you're getting some CSS that appears to be intended for Safari suggests to me that either Google has a bug on their end that's somehow sending bad CSS to Camino, or you're spoofing your user-agent in some manner that makes Google think you're using Safari.

Can you try it with a fresh profile and see if you still see the same problem?

Comment 3

[Stuart Morgan]

(In reply to comment #2)
> The fact that you're getting some CSS that appears to be intended for Safari
> suggests to me that either Google has a bug on their end that's somehow sending
> bad CSS to Camino, or you're spoofing your user-agent in some manner that makes
> Google think you're using Safari.

Lots of sites use vendor-specific tags in CSS they send to everyone, since browsers will ignore CSS they don't understand, by spec.

Comment 4

[philippe (part-time)]

All those entries about css 'errors' are just warnings (and most refer to Win IE stuff).
The real question: does Google Reader work correctly with the latest Minefield build(s) ? Iirc there was a comment on the forums from someone with problems about it.

Comment 5

[cannedbrain+mozilla]

(In reply to comment #2)
> The fact that you're getting some CSS that appears to be intended for Safari
> suggests to me that either Google has a bug on their end that's somehow sending
> bad CSS to Camino, or you're spoofing your user-agent in some manner that makes
> Google think you're using Safari.
> 
> Can you try it with a fresh profile and see if you still see the same problem?
> 

I don't spoof my user-agent. It works fine with the latest branch, like I said. I tried it with a fresh profile and the problem still persisted. On a side note, the page appearance (the tabs on top of the page) is also messed up in the latest trunk:

http://cannedbrain.imap.cc/branch.png
http://cannedbrain.imap.cc/trunk.png

Comment 6

[Stuart Morgan]

I can't repro this with trunk; adding tags works fine for me. Does anyone else see it?

Comment 7

[philippe (part-time)]

(In reply to comment #6)

Yes I could reproduce it (testing on the BBC-world feed). 
STR
Scroll down the view list,
try adding a tag to one of the bottom items/articles.
result
The blue box with the input field is about 150px down from the selected item (which is scrolled out of view), and the auto-complete box some 200~300px lower.

I checked with Minefield just to be sure. It has the same issue as Camino.

The page appearance issue noted in comment 5 is a known issue with absolute positioning, floated tables (and is not Camino specific either): bug 370248.

Comment 8

[philippe (part-time)]

This regressed between Minefield builds 20070530 Minefield/3.0a5pre and 20070531 Minefield/3.0a5pre

http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2007-05-30+03%3A00%3A00&maxdate=2007-05-31+04%3A00%3A00&cvsroot=%2Fcvsroot

Possibly a regression from 174397 ?

Comment 9

[Martijn Wargers (dead)]

Yeah, this could very well be a regression somehow from bug 174397.

I found this code in the Google reader:
Nd=function(a){var b=Xc(a);if(!ka(ik)){ik=hr&&!(jr(ue,MB)>=0)}var c=new Na(0,0),d=Yg(b);if(a==d){return c}var e=null,f;if(a.getBoundingClientRect){f=a.getBoundingClientRect();var g=d.scrollTop,h=d.scrollLeft;c.x=f.left+h;c.y=f.top+g}else if(b.getBoxObjectFor&&!ik){f=b.getBoxObjectFor(a);var i=b.getBoxObjectFor(d);c.x=f.screenX-i.screenX;c.y=f.screenY-i.screenY}else{c.x=a.offsetLeft;c.y=a.offsetTop;e=a.offsetParent;
if(e!=a){while(e){c.x+=e.offsetLeft;c.y+=e.offsetTop;e=e.offsetParent}}if(hh||tf&&kk(a,NB)==Tl){c.y-=b.body.offsetTop}e=a.parentNode;while(e&&e!=d){c.x-=e.scrollLeft;if(!hh||e.tagName!=OB){c.y-=e.scrollTop}e=e.parentNode}}return c}

So in current trunk build the getBoundingClientRect path is followed in this case.
Not sure if there is a bug in Firefox here or with the website code. I suspect the latter, though.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The good news is that it's not taking the getBoxObjectFor path!

Do we have any hope for figuring out what this code is doing and/or why it's failing?

Comment 11

[Martijn Wargers (dead)]

Attachment: testcase

Ok, apparently this is a bug in Mozilla.
Mozilla doesn't take scrolling into account at all for getBoundingClientRect. Probably the same problem with getClientRects().

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Oh, so IE does take scrolling into account? I was under the impression that it did not.

Comment 13

[Martijn Wargers (dead)]

(In reply to comment #12)
> Oh, so IE does take scrolling into account? I was under the impression that it
> did not.

Yes, IE takes scrolling into account. The values can even become negative because of that.

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

alright then, this is a super-blocker

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Does it take scrolling of the viewport into account?

Comment 16

[Martijn Wargers (dead)]

Yes (will attach a testcase later).

Comment 17

[Martijn Wargers (dead)]

Attachment: testcase2

Testcase sucks quite a bit, but it shows that the viewport scrolling is taken into account.

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

This should do it.

Can someone verify that it fixes Google Reader? I'd rather not set it up just for this?

Comment 19

[Martijn Wargers (dead)]

Yeah, that patch fixes the Google Reader issue for me.

Comment 20

[philippe (part-time)]

Testing that patch on OS X: Google Reader works just fine now.

Comment 21

[NomenNescio]

Could someone confirm if this also fixes the behaviour described
here: http://forums.mozillazine.org/viewtopic.php?t=579385 ?

Comment 22

[Boris Zbarsky [:bzbarsky]]

So this does mean that the values returned from this function don't match up with client coords (as in getElementFromPoint and event.client*) due to taking viewport scrolling into account?

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

elementFromPoint and event.client* take viewport scrolling into account, so actually we're making them consistent.

Comment 24

[Boris Zbarsky [:bzbarsky]]

Ah, ok.  Most excellent.

Comment 25

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

if roc says it's a super-blocker, then it is :) Especially since it's already assigned to him.

Comment 26

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 278866 [details] [diff] [review]
fix

r+sr+a1.9=dbaron

Comment 27

[Robert O'Callahan (:roc) (email my personal email if necessary)]

checked in

Comment 29

[Eric Shepherd [:sheppy]]

What if anything about this is supposed to be documented?  It appears to simply be about bug fixes to a function that was already new in Fx3 anyway. :)

Comment 30

[Eric Shepherd [:sheppy]]

OK, comment 23 is the thing that needs documenting -- scrolling is taken into account for get(Bounding)ClientRect(s).

Comment 31

[Eric Shepherd [:sheppy]]

I've added text to the articles for both functions mentioning that scrolling is considered.