Last Comment Bug 670467 - micromedex 2.0 doesn't work properly with firefox 4 and later
: micromedex 2.0 doesn't work properly with firefox 4 and later
Status: RESOLVED FIXED
: regression
Product: Core
Classification: Components
Component: General (show other bugs)
: Trunk
: All All
: P1 normal (vote)
: mozilla8
Assigned To: Boris Zbarsky [:bz] (still a bit busy)
:
:
Mentors:
http://www.thomsonhc.com/
Depends on: 678929
Blocks: 494117
  Show dependency treegraph
 
Reported: 2011-07-09 13:51 PDT by Mjules
Modified: 2011-08-14 20:14 PDT (History)
6 users (show)
bzbarsky: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Minimal-ish testcase (378 bytes, text/html)
2011-07-13 11:53 PDT, Boris Zbarsky [:bz] (still a bit busy)
no flags Details
Testcase showing the same problem with continuations when the abovementioned optimization doesn't happen (430 bytes, text/html)
2011-07-13 19:19 PDT, Boris Zbarsky [:bz] (still a bit busy)
no flags Details
Proposed fix (8.85 KB, patch)
2011-07-13 21:14 PDT, Boris Zbarsky [:bz] (still a bit busy)
dbaron: review+
Details | Diff | Splinter Review

Description Mjules 2011-07-09 13:51:05 PDT
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 2 Boris Zbarsky [:bz] (still a bit busy) 2011-07-11 07:08:19 PDT
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 2011-07-11 13:13:55 PDT
Hi, 

the saved page does not show the issue.

I contact you in private for the access.
Comment 4 Boris Zbarsky [:bz] (still a bit busy) 2011-07-12 13:34:22 PDT
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 [:bz] (still a bit busy) 2011-07-13 11:53:50 PDT
Created attachment 545715 [details]
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 [:bz] (still a bit busy) 2011-07-13 11:56:57 PDT
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 [:bz] (still a bit busy) 2011-07-13 12:40:34 PDT
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 2011-07-13 12:54:05 PDT
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 [:bz] (still a bit busy) 2011-07-13 19:19:34 PDT
Created attachment 545815 [details]
Testcase showing the same problem with continuations when the abovementioned optimization doesn't happen
Comment 10 Boris Zbarsky [:bz] (still a bit busy) 2011-07-13 21:14:26 PDT
Created attachment 545824 [details] [diff] [review]
Proposed fix
Comment 11 Boris Zbarsky [:bz] (still a bit busy) 2011-07-13 21:15:41 PDT
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 [:bz] (still a bit busy) 2011-07-13 21:20:18 PDT
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 2011-07-14 01:14:52 PDT
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 [:bz] (still a bit busy) 2011-07-14 06:35:50 PDT
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 [:bz] (still a bit busy) 2011-07-14 06:36:13 PDT
And to be clear, I really appreciate you filing this now, and not in early 2012.  ;)
Comment 16 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2011-08-11 15:29:19 PDT
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 [:bz] (still a bit busy) 2011-08-11 17:00:13 PDT
> 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 [:bz] (still a bit busy) 2011-08-11 20:54:24 PDT
> 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) 2011-08-12 07:17:56 PDT
https://hg.mozilla.org/mozilla-central/rev/77a0d9f09820

Note You need to log in before you can comment on or make changes to this bug.