670467 - micromedex 2.0 doesn't work properly with firefox 4 and later

Status: RESOLVED FIXED

(Core :: General, defect, P1)

Description

[Mjules]

Hi,

I'm a user of the site micromedex ( http://www.thomsonhc.com/ ) and the 2.0 version doesn't work properly with firefox 4 and later.


Description of the problem :

when I do a search, results are not displayed. I get a blank zone instead of result zone.

If I display the html source code, results are here, confirmed by the fact than if I choose no style in view/Page style, I saw the results.

More surprising, if I go to no style and come back to default style, I got the results displayed. The same if I use a function of the site which display a popup, magically, results appear. The same with zoom in/zoom out.

here is a demonstration of the site (interesting part between 0:44 and 1:12) :
http://www.youtube.com/watch?v=Dwsm5PsQyQU

and here is a demonstration of the bug :
http://www.youtube.com/watch?v=ryCDCryik2M


I've pseudo-bisected the problem to a commit between 18/6/2010 and 19/6/2010 on mozilla central

commit ID between 
https://hg.mozilla.org/mozilla-central/rev/979b65e6b73e 
and
https://hg.mozilla.org/mozilla-central/rev/befec51e10ab

I can reproduce with firefox 5.0 on windows XP and aurora from today on linux

thanks

Comment 1

[Thomas Ahlblom]

Link to regression range in comment 0:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=979b65e6b73e&tochange=befec51e10ab

Comment 2

[Boris Zbarsky [:bzbarsky]]

Looks like a likely regression from bug 494117 or bug 479655.

Unfortunately, it's hard to say more than that without being able to get at the page......

Mjules, if you save the page that shows the problem as "web page, complete", does that saved page show the issue?  If not, is there any way I can get access to the page that shows the problem?

Comment 3

[Mjules]

Hi, 

the saved page does not show the issue.

I contact you in private for the access.

Comment 4

[Boris Zbarsky [:bzbarsky]]

OK, this is definitely a regression from bug 494117.  If I just change ReResolveStyleContext to reresolve all kids instead of reparenting them, things start to work.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Minimal-ish testcase

The key here is toggling visibility on the inline of an {ib} split and at the same time also toggling style on an ancestor of that inline (the site changes the body's cursor style, but color works too).

Comment 6

[Boris Zbarsky [:bzbarsky]]

In the meantime, a way to work around the problem is to use this bookmarklet on the site:

javascript:var ___=document.getElementById("contentLeft").style;___.visibility="hidden";___.visibility="visible"

Comment 7

[Boris Zbarsky [:bzbarsky]]

OK, so the basic issue is that we end up not actually reresolving the ib special siblings because by the time we get there the restyle tracker no longer has an entry for that element.  So we just reparent them, which is buggy.

This would probably bite us with regular continuations too, except for the copyFromContinuation optimization.  If I disable that optimization I get the bug there too.

Comment 8

[Mjules]

Thank you !

In the meantime, I took the liberty to make a small userscript from your bookmarklet in order to use it with greasemonkey or scriptish.
http://mjules.free.fr/script/firemedex.user.js

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase showing the same problem with continuations when the abovementioned optimization doesn't happen

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed fix

Comment 11

[Boris Zbarsky [:bzbarsky]]

David, when you review, could you please let me know whether you want this on any of the branches?  My gut feeling is that we don't want it on 6, given how long ago this broke, but may want it on 7....

Comment 12

[Boris Zbarsky [:bzbarsky]]

Or may not, given that this is the first report of this problem in the more than a year since it appeared and this code is a bit finicky in the past.

Comment 13

[Mjules]

FWIW, It has been reported on mozilla.feedback in May (when micromedex v2.0 began to be advertised) :
http://groups.google.com/group/mozilla.feedback.firefox/browse_thread/thread/11eef89718abfb42

micromedex 2.0 will be mandatory at the beginning of 2012, in the meantime, we can always use 1.0 which works fine or use the workaround provided.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Yeah, beginning of 2012 is not a problem.  If this lands in the next several weeks, it will ship in Firefox 8 in November.

Comment 15

[Boris Zbarsky [:bzbarsky]]

And to be clear, I really appreciate you filing this now, and not in early 2012.  ;)

Comment 16

[David Baron :dbaron:]

Comment on attachment 545824 [details] [diff] [review]
Proposed fix

>Bug 670467.  Correctly reresolve style on non-first continuations and non-first parts of {ib} splits even if we're just reparenting the first continuation of first part of the {ib} split.  r=dbaron

I think the word "of" in this line should be "or".  At least that
makes it make a lot more sense to me given comment 7.

>+  // Account for {ib} splits when looking for "prevContinuation".  In
>+  // particular, for the first-continuation of a part of an {ib} split we
>+  // want to use the special prevsibling of the special prevsibling of
>+  // aFrame, which should have the same style context as aFrame itself

I presume this is because you only want to do this for an inline part of
an {ib} split and this is in order to get to the previous inline part 
and skip over a block part.  Could you actually explain that in the 
comment -- and then point to that comment from the other places where
you do the double-traversal?

(I presume {ib} splits still maintain the invariant that there's exactly
one block between a pair of inlines, and never more.)

r=dbaron with that

Comment 17

[Boris Zbarsky [:bzbarsky]]

> I think the word "of" in this line should be "or". 

Yes.  Will fix.

> I presume this is because you only want to do this for an inline part of
> an {ib} split

I want to do it for block parts too, but I want to do the double-traversal for those as well.  I'll fix the comment to make that clearer.

> I presume {ib} splits still maintain the invariant that there's exactly
> one block between a pair of inlines

Yes, they do.

Comment 18

[Boris Zbarsky [:bzbarsky]]

> Yes.  Will fix.

Actually, the checkin comment just didn't make sense.  I've rewritten it to make sense now.

http://hg.mozilla.org/integration/mozilla-inbound/rev/77a0d9f09820

Comment 19

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/77a0d9f09820