581536 - Resizer direction doesn't handle dynamic changes correctly.

Status: RESOLVED FIXED

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

Description

[(no longer active)]

STR:

1. Load: data:text/html,<textarea>
2. Paste this code in the URL bar:

javascript:void(document.querySelector("textarea").setAttribute("dir","rtl"))

The problem is that we only set the direction in the resizer's ctor: <http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/resizer.xml#15>

Comment 1

[(no longer active)]

Is it possible to reconstruct the XBL bound to an element based on a style change?

Comment 2

[(no longer active)]

This is seen on Google Translate, BTW.

Comment 3

[Boris Zbarsky [:bzbarsky]]

> Is it possible to reconstruct the XBL bound to an element based on a style
> change?

Not per se.

However if we do start reframing on direction change (as proposed in bug 563665), this will Just Work, since a reframe will reinstantiate the binding.

Comment 4

[Dave Townsend [:mossop]]

I don't believe that this blocks as it only impacts a small number of sites.

Comment 5

[Simon Montagu :smontagu]

If bidi UI is enabled (which it is by default in RTL localizations), this impacts every site in the world, since the user can change the direction of any text area from the context menu or by a keyboard shortcut, and it is very common to do so.

Comment 6

[Tomer Cohen :tomer]

This bug is very common in Gmail chat feature, as they change the textarea direction automatically for every paragraph/message as the user mix between LTR and RTL characters.

Comment 7

[Dave Townsend [:mossop]]

(In reply to comment #5)
> If bidi UI is enabled (which it is by default in RTL localizations), this
> impacts every site in the world, since the user can change the direction of any
> text area from the context menu or by a keyboard shortcut, and it is very
> common to do so.

Thanks for the extra details, sounds like this blocks then.

Comment 8

[Yaron Shahrabani]

This but still exist in Firefox 4, I just checked it with Gmail
Steps to reproduce:
1. Open your Gmail (if you have one)
2. Go to Buzz (if its not disabled yet)
3. Click on the Comment on one of the Buzzes there (if you have any)
4. In the comment textbox paste the following text: שלום

The textbox will turn RTL, now the corner moves to the left but stays exactly the same meaning that instead of pointing outside the box it will point away from the corner, so as the mouse pointer (pointing towards inside the box while it should point the other way).

Comment 9

[Tomer Cohen :tomer]

Just to make sure people understand the situation Yaron mentioned - Gmail use input widgets that change direction automatically based on content (first strong letter, probably).

His steps to reproduce are longer than just typing ctrl-shift-x on any textarea (including bugzilla's comment box), but more trivial to users, as most of them are not bothered by directionality and like the idea that google does it for them automatically. 

Marking as Platform:All, as we can reproduce this issue on Linux and Windows as well.

Comment 10

[(no longer active)]

Attachment: Patch (v1)

Comment 11

[Boris Zbarsky [:bzbarsky]]

Do we want to restrict to @dir changes like that?  What about changes to inline style or the set of matching CSS rules that lead to a computed direction change?

Comment 12

[(no longer active)]

Attachment: Patch (v2)

(In reply to comment #11)
> Do we want to restrict to @dir changes like that?  What about changes to inline
> style or the set of matching CSS rules that lead to a computed direction
> change?

You're completely right.  We should handle it at the style level.

Comment 13

[(no longer active)]

Although, I'm not sure whether the CalcDifference changes could only be restricted to textarea elements...

Comment 14

[Boris Zbarsky [:bzbarsky]]

> Although, I'm not sure whether the CalcDifference changes could only be
> restricted to textarea elements...

Not easily without undoing a bunch of optimizations the style system does.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 497320 [details] [diff] [review]
Patch (v2)

No need to test mDirection twice; use else if as needed.

You need to fix MaxDifference for that struct, I think.  It looks like it's already in the right place in nsStyleContext::CalcStyleDifference, oddly.

Comment 16

[(no longer active)]

Attachment: Patch (v2.1)

(In reply to comment #15)
> Comment on attachment 497320 [details] [diff] [review]
> Patch (v2)
> 
> No need to test mDirection twice; use else if as needed.

Heh, right!  This function is too confusing :(

> You need to fix MaxDifference for that struct, I think.  It looks like it's
> already in the right place in nsStyleContext::CalcStyleDifference, oddly.

I'm not sure what you mean.  I did patch nsStyleVisibility::MaxDifference to take ReconstructFrame into account (and I realized that I needed to do that because of the assertion that I got from nsStyleContext::CalcStyleDifference).

Do I need to do anything else here?

Comment 17

[Boris Zbarsky [:bzbarsky]]

> I did patch nsStyleVisibility::MaxDifference

Er, so you did.  I have no idea how I missed that!

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 497543 [details] [diff] [review]
Patch (v2.1)

The mLanguage check is overparenthesized.  Fix that and r=me.

Comment 19

[(no longer active)]

Attachment: For check-in

Comment 20

[(no longer active)]

So, with this patch, setting |document.dir = "rtl"| leads to crashes like this:

http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1292406318.1292407211.19016.gz&fulltext=1

Because we expect to have an nsIContent* for frame reconstruction, and we don't get one for viewport frames.

Boris suggested that we should always set the direction on the viewport frame's style context to LTR.  I tried that, but that messes up the visibility style inheritance.  I'm trying to figure out if I can work around it somehow...

Comment 21

[(no longer active)]

So I tried reconstructing frames for the document element and all of its descendants, but the inheritance is still broken, which causes the frame reconstruction to not do the right thing (although it avoids the crash).

Boris, do you have any ideas how to approach this?  We may need to just deal with the nsIContent pointer being null... :(

Comment 22

[(no longer active)]

Attachment: Part 2: reconstruct the document element frame correctly when the dir property on the document is set

So, I think this is the right fix.  Boris, what do you think?

Comment 23

[(no longer active)]

Attachment: Part 3: Fix a bogus debug check

There was an abort in browser/base/content/test/tabview/browser_tabview_rtl.js because of a debug check, triggered by the patches on this bug.  This patch fixes the check.

Comment 24

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 497859 [details] [diff] [review]
Part 2: reconstruct the document element frame correctly when the dir property on the document is set

r=me

Comment 25

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 498050 [details] [diff] [review]
Part 3: Fix a bogus debug check

dbaron is a better reviewer for this

Comment 26

[(no longer active)]

Comment on attachment 498050 [details] [diff] [review]
Part 3: Fix a bogus debug check

Sorry for flipping the switch, missed a bugmail!

Comment 27

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

Comment on attachment 498050 [details] [diff] [review]
Part 3: Fix a bogus debug check

Well, I wouldn't say it's a bogus check; it's just guaranteed not to fire if we take the else branch, so it can safely be moved into the if, at least based on the current structure of the code.

r=dbaron, although I have mixed feelings.  It might be nice to leave the assertion checking this in all cases in case the structure of the code changes, since the proof that this is ok requires reading a substantial chunk of code.

Comment 28

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

Comment on attachment 498050 [details] [diff] [review]
Part 3: Fix a bogus debug check

Actually, if you're changing this because it actually fired, then review-.  It shouldn't have fired; it seems like the way currentIndex is set we should be able to prove that this patch can't help.

Are you sure you weren't seeing bug 611238?

Comment 29

[(no longer active)]

Attachment: Part 3: Disconnect the transaction manager from the pres context in time

I sat down with dbaron today to debug this.  What's happening is that we're disconnecting the transition manager from the pres context in its destructor, which means that if the pres context lives on longer after its SetShell(null) method has been called, we won't delete the transition properties from the element.

This patch fixes this issue.

Comment 30

[(no longer active)]

Attachment: Debugging patch

This is the debugging patch that we used to track down this issue.  I'm attaching it here in case somebody needs to look at it in the future.

Comment 31

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

Comment on attachment 498485 [details] [diff] [review]
Part 3: Disconnect the transaction manager from the pres context in time

>Bug 581536 - Part 3: Disconnect the transaction manager from the pres context as soon as it's disconnected from the pres shell

s/it's disconnected from the pres shell/the presentation is torn down/

>This patch ensures that if for some reason, the pres context for a pres shell
>goes away, we do not hold on to the old transaction manager.  Doing so would
>lead into the transition properties not being deleted from the elements, which
>will later on confuse the new transaction manager if we ever create one.

I'd replace this sentence with:

This patch ensures that we tell the transition manager to shut itself down (and delete its properties on elements) when the presentation is destroyed instead of waiting until the pres context's destructor runs.  The pres context's destructor might not run until a new presentation is created, and that new presentation's transition manager could be confused by these dangling properties containing transitions from the old presentation.

r=dbaron

Comment 32

[(no longer active)]

Note to myself: this seems to be causing a lot of crashes, such as:

http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1292655575.1292655761.2141.gz&fulltext=1
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1292655576.1292655948.2919.gz&fulltext=1
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1292655575.1292655719.2060.gz&fulltext=1
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1292655576.1292655999.3269.gz&fulltext=1

I will probably get to look at this again once I'm back.

Comment 33

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

Maybe XSLT stuff reuses the same pres context.

SetShell should probably null out the transition manager.  Then, depending on whether the XSLT stuff calls Init again, we might need to create a new transition manager somewhere.

Comment 34

[Boris Zbarsky [:bzbarsky]]

XSLT calls SetDOMDocument on the document viewer, which will land in DocumentViewerImpl::SetDocumentInternal.  It looks like this will in fact destroy the presshell but reuse the prescontext.  <sigh>

Jonas, can we just make SetDOMDocument fully reinit the document viewer instead or something?

Comment 35

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

I've never understood how this code works and as far as I know it's remained mostly untouched as long as, well, forever.

I wouldn't assume that the code does anything for any particular reasons that are still valid.

And, very importantly, I think this is one of very few places where the presshell and prescontext lifetimes differ, possibly the only such place, and thus stand in the way of the two of them merging.

So in short, would be awesome if someone could change this code to not reuse the prescontext and see if that breaks anything.

Comment 36

[Boris Zbarsky [:bzbarsky]]

OK, sold.  Ehsan, you want to be someone?  Or should I try to do it?  Or maybe Timothy?

Might be worth working around it for now then doing the real fix in a separate bug (post-2.0?) and removing the workaround then.  :(

Comment 37

[(no longer active)]

I tried setting mPresContext to null in ContentViewerImpl::SetDocumentInternal, but it seems to break XSLT completely.

I don't really have any idea what needs to be done here.  Can someone provide some tips (or better yet, take a stab at it!)?

Thanks!

Comment 38

[Boris Zbarsky [:bzbarsky]]

Did you add something to create a new prescontext too?  If not, then of course things broke....

I can probably take a stab at this next week.

Comment 39

[(no longer active)]

(In reply to comment #38)
> Did you add something to create a new prescontext too?  If not, then of course
> things broke....

I tried calling InitPresentationStuff, to no avail.

> I can probably take a stab at this next week.

Great, thanks!

Comment 40

[Boris Zbarsky [:bzbarsky]]

Do you want to reassign to me, then?

Comment 41

[(no longer active)]

(In reply to comment #40)
> Do you want to reassign to me, then?

Sure.  Thanks!

Comment 42

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix the XSLT issues

Comment 43

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

(In reply to comment #13)
> Although, I'm not sure whether the CalcDifference changes could only be
> restricted to textarea elements...

BTW, for the record, they could, by overriding nsIFrame::DidSetStyleContext for nsTextControlFrame.

Comment 44

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

Also, when you reland, please remember to change this:

(In reply to comment #33)
> SetShell should probably null out the transition manager.

even though it may not be needed with the new patch.

Comment 45

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

Comment on attachment 505666 [details] [diff] [review]
Fix the XSLT issues

In the *declaration* in nsDocumentViewer.cpp, you should make the 
parameter name match the name in the comment and in the definition
(i.e., change aSetNewDocument to aForceSetNewDocument).

In SetDocumentInternal, are you assuming that !mPresShell == 
!mPresContext?  It sort of seems like you might be; it might be worth
asserting, especially if the assertion doesn't trigger. :)

r=dbaron with that

Comment 46

[Boris Zbarsky [:bzbarsky]]

> you should make the parameter name match

Oops, yes.  Will fix.

> are you assuming that !mPresShell == !mPresContext?

No, I'm not.  I believe the only invariant that can be assumed here is that |!mPresContext| implise |!mPresShell|....

Comment 47

[Boris Zbarsky [:bzbarsky]]

Attachment: Address dbaron's comments on XSLT bits

Comment 48

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/f12f55c744f4
http://hg.mozilla.org/mozilla-central/rev/f3432e1de085
http://hg.mozilla.org/mozilla-central/rev/80df95979d3a
http://hg.mozilla.org/mozilla-central/rev/7294ab636e1d

Comment 49

[Aleksei Perepelkin [:chaos]]

Resizer position changes but in RTL-mode it becomes invisible.

Tested on Mozilla/5.0 (X11; Linux i686; rv:2.0b12pre) Gecko/20110204 Firefox/4.0b12pre

Comment 50

[u279076]

(In reply to comment #49)
> Resizer position changes but in RTL-mode it becomes invisible.
> 
> Tested on Mozilla/5.0 (X11; Linux i686; rv:2.0b12pre) Gecko/20110204
> Firefox/4.0b12pre

This might be a new bug, but lets reopen just in case.

Comment 51

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

Are you seeing bug 519152?

Comment 52

[(no longer active)]

This bug its original form is now fixed.  Please file a new bug with exact steps to reproduce if you see the problem in comment 49, and CC me on it.

Thanks!

Comment 53

[u279076]

(In reply to comment #52)
> This bug its original form is now fixed.  Please file a new bug with exact
> steps to reproduce if you see the problem in comment 49, and CC me on it.
> 
> Thanks!

Aleksej, can you take care of that since you reported the RTL issue?  Thanks.

Comment 54

[Aleksei Perepelkin [:chaos]]

(In reply to comment #51)
> Are you seeing bug 519152?

Yeah, looks like it. So...

Verified fixed with Mozilla/5.0 (X11; Linux i686; rv:2.0b12pre) Gecko/20110204 Firefox/4.0b12pre