Closed Bug 793233 Opened 7 years ago Closed 7 years ago

bidi reftest 779003, 712600 broken with dynamic changes


(Core :: Layout: Text and Fonts, defect)

Not set



Tracking Status
firefox17 + verified
firefox18 + verified


(Reporter: vlad, Assigned: smontagu)




(6 files)

I'm actually hitting this with the patch in bug 731974 that changes the way nsRefreshDriver's timers work, but I discovered that the "wrong" rendering also happens on nightly if the reftest content is dynamically changed in a trivial way.  The attached testcase is a slightly modified version of the reftest, that just removes a trivial <li> node a second after the page loads.  The rendering completely changes in a nightly build, and ends up looks like the "broken" case after my patch.
This is interesting:  I'm used to seeing dynamic invalidation bugs where forcing a reflow or forcing a frame reconstruct makes the bug go away, and then once it's gone it stays gone.

This one is unusual in that forcing a reflow and forcing a frame reconstruct *both* cause a state change if the most recent change was the other one.  In particular, if I open Web console, and alternately do:'30px';getComputedStyle(document.body, "").width;'''none';getComputedStyle(document.body, "").width;''

I end up switching between the two states.
David, Simon, are either/both of you ok with marking this reftest as random for now?
Attachment #665529 - Flags: review?(smontagu)
Hrm.  Other tests fail in this same way -- dynamic version of 712600-1.
Blocks: 779003, 712600
Summary: reftest 779003-1.html broken with dynamic changes → bidi reftest 779003, 712600 broken with dynamic changes
Did you mean to set this as tracking?
Comment on attachment 665529 [details] [diff] [review]
mark this reftest as random

Review of attachment 665529 [details] [diff] [review]:

OK, mark the tests random for now, then leave the bug open and assign it to me for further investigation.
Attachment #665529 - Flags: review?(smontagu) → review+

Please add [leave open] to the whiteboard when you want a bug left open so our merge tools don't auto-resolve it.
Assignee: nobody → vladimir
Whiteboard: [leave open]
Assignee: vladimir → smontagu
Whoops, I didn't actually press Save Changes when I did all that to the bug.. sorry!
Assignee: smontagu → bugs
Assignee: bugs → smontagu
Clearing the tracking-firefox18 flag, as I think it was set by mistake ?!?! Please renominate if it needs to be tracked .
Hm, not sure why it looked like a mistake?  It's an issue that affects fx18 and is potentially a regression; it might be tracking- if we don't care, but it was nominated on purpose...
Hey Vlad, apologies for not being clear on my comment 9 . 
Reframing it here & leaving it open for nomination :
It will be very helpful if we can have the user impact outlined to track it for release . Was not sure if the rendering impact affected our users visibly or was it only the tests ?
Simon may be able to describe user impact better, but it seems like it could be an issue that would affect any sites doing RTL and dynamic html manipulation.. they would end up with fairly broken rendering.
Simon, can you please help with some input on comment 12 ?It will give us better idea to decide if we need to track it for release.  Thanks !
Also do we know of any URL's where the impact can be seen ?
I don't know for sure if this has user-facing impact, but similar bugs (e.g. bug 731594) have caused backward text on Facebook, so I would rather play safe and track this for release.
Attached patch FixSplinter Review
The bug here even has a comment justifying it, but I didn't think it through properly: it's true that SplitInlineAncestors doesn't blow up with a null parameter, but in this context it doesn't make sense to call it if we don't have a previous sibling: the point of this code is to split runs of text with bidi isolation in separate frames, so if there is no previous sibling there is nothing to split, and we end up creating an empty inline. Then since the inline has no text frame containees with embeddingLevel property, reordering gets messed up.
Attachment #675005 - Flags: review?(roc)
Attached patch TestsSplinter Review
This doesn't include the dynamic version of 712600-1, since AFAICT the difference in rendering after deleting LTR text at the beginning of an RTL paragraph is expected.
Attachment #675006 - Flags: review?(roc)
Flags: in-testsuite+
OS: Windows 7 → All
Hardware: x86_64 → All
Whiteboard: [leave open]
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Saw the issue on 19.0a1 (2012-10-24).
Verified fixed on 19.0a1 (2012-10-28) on Win 7, Ubuntu 12.04 and Mac OS X 10.7.5
(In reply to Simon Montagu from comment #18)

Please nominate for aurora uplift after enough bake time along with risk assessment once ready.Thanks !
Comment on attachment 675005 [details] [diff] [review]

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 712600
User impact if declined: see comment 14 and dependent bug 767765
Testing completed (on m-c, etc.): Baked on trunk since 2012-10-25
Risk to taking this patch (and alternatives if risky): This is an area where there have been a number of fixes and regressions, so there is some risk, but I think it's acceptably small.
String or UUID changes made by this patch: None
Attachment #675005 - Flags: approval-mozilla-beta?
Attachment #675005 - Flags: approval-mozilla-aurora?
Comment on attachment 675005 [details] [diff] [review]

Please uplift today to branches so that this is in tomorrow's beta.
Attachment #675005 - Flags: approval-mozilla-beta?
Attachment #675005 - Flags: approval-mozilla-beta+
Attachment #675005 - Flags: approval-mozilla-aurora?
Attachment #675005 - Flags: approval-mozilla-aurora+
(In reply to Paul Silaghi [QA] from comment #20)
> Saw the issue on 19.0a1 (2012-10-24).
> Verified fixed on 19.0a1 (2012-10-28) on Win 7, Ubuntu 12.04 and Mac OS X
> 10.7.5
Verified fixed on FF 17b5, FF 18.0a2 (2012-11-13).
Blocks: 775687
You need to log in before you can comment on or make changes to this bug.