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

RESOLVED FIXED in mozilla8

Status

()

Core
General
P1
normal
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Mjules, Assigned: bz)

Tracking

({regression})

Trunk
mozilla8
regression
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(3 attachments)

(Reporter)

Description

6 years ago
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

6 years ago
Link to regression range in comment 0:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=979b65e6b73e&tochange=befec51e10ab
Version: unspecified → 7 Branch
Component: General → General
Keywords: regression
Product: Firefox → Core
QA Contact: general → general
Version: 7 Branch → Trunk
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?
(Reporter)

Comment 3

6 years ago
Hi, 

the saved page does not show the issue.

I contact you in private for the access.
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.
Assignee: nobody → bzbarsky
Blocks: 494117
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P1
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).
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"
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.
(Reporter)

Comment 8

6 years ago
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
Created attachment 545815 [details]
Testcase showing the same problem with continuations when the abovementioned optimization doesn't happen
Created attachment 545824 [details] [diff] [review]
Proposed fix
Attachment #545824 - Flags: review?(dbaron)
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....
Whiteboard: [need review]
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.
(Reporter)

Comment 13

6 years ago
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.
Yeah, beginning of 2012 is not a problem.  If this lands in the next several weeks, it will ship in Firefox 8 in November.
And to be clear, I really appreciate you filing this now, and not in early 2012.  ;)
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
Attachment #545824 - Flags: review?(dbaron) → review+
> 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.
> 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
Flags: in-testsuite+
Whiteboard: [need review]
Target Milestone: --- → mozilla8
https://hg.mozilla.org/mozilla-central/rev/77a0d9f09820
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED

Updated

6 years ago
Depends on: 678929
You need to log in before you can comment on or make changes to this bug.