Closed Bug 581536 Opened 14 years ago Closed 13 years ago

Resizer direction doesn't handle dynamic changes correctly.

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla2.0b10
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: ehsan.akhgari, Assigned: ehsan.akhgari)

References

Details

(Keywords: rtl, Whiteboard: [softblocker][fx4-fixed-bugday])

Attachments

(5 files, 5 obsolete files)

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>
Keywords: rtl
Is it possible to reconstruct the XBL bound to an element based on a style change?
blocking2.0: --- → ?
This is seen on Google Translate, BTW.
> 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.
I don't believe that this blocks as it only impacts a small number of sites.
blocking2.0: ? → -
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.
blocking2.0: - → ?
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.
(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.
blocking2.0: ? → betaN+
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).
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.
OS: Mac OS X → All
Hardware: x86 → All
Attached patch Patch (v1) (obsolete) — Splinter Review
Assignee: nobody → ehsan
Status: NEW → ASSIGNED
Attachment #496991 - Flags: review?(bzbarsky)
Component: XUL Widgets → DOM: Core & HTML
Product: Toolkit → Core
QA Contact: xul.widgets → general
Blocks: 167951
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?
Attached patch Patch (v2) (obsolete) — Splinter Review
(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.
Attachment #496991 - Attachment is obsolete: true
Attachment #497320 - Flags: review?(bzbarsky)
Attachment #496991 - Flags: review?(bzbarsky)
Although, I'm not sure whether the CalcDifference changes could only be restricted to textarea elements...
> 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 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.
Attachment #497320 - Flags: review?(bzbarsky) → review-
Attached patch Patch (v2.1) (obsolete) — Splinter Review
(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?
Attachment #497320 - Attachment is obsolete: true
Attachment #497543 - Flags: review?(bzbarsky)
> I did patch nsStyleVisibility::MaxDifference

Er, so you did.  I have no idea how I missed that!
Comment on attachment 497543 [details] [diff] [review]
Patch (v2.1)

The mLanguage check is overparenthesized.  Fix that and r=me.
Attachment #497543 - Flags: review?(bzbarsky) → review+
Attached patch For check-inSplinter Review
Attachment #497736 - Attachment is patch: true
Attachment #497736 - Attachment mime type: application/octet-stream → text/plain
Attachment #497543 - Attachment is obsolete: true
Whiteboard: [needs landing]
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...
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... :(
So, I think this is the right fix.  Boris, what do you think?
Attachment #497859 - Flags: review?(bzbarsky)
Attached patch Part 3: Fix a bogus debug check (obsolete) — Splinter Review
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.
Attachment #498050 - Flags: review?(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
Attachment #497859 - Flags: review?(bzbarsky) → review+
Comment on attachment 498050 [details] [diff] [review]
Part 3: Fix a bogus debug check

dbaron is a better reviewer for this
Attachment #498050 - Flags: review?(bzbarsky) → review?(dbaron)
Attachment #498050 - Flags: review?(dbaron) → review?(bzbarsky)
Comment on attachment 498050 [details] [diff] [review]
Part 3: Fix a bogus debug check

Sorry for flipping the switch, missed a bugmail!
Attachment #498050 - Flags: review?(bzbarsky) → review?(dbaron)
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.
Attachment #498050 - Flags: review?(dbaron) → review+
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?
Attachment #498050 - Flags: review+ → review-
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.
Attachment #498485 - Flags: review?(dbaron)
Attached patch Debugging patchSplinter Review
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.
Whiteboard: [needs landing] → [needs review dbaron]
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
Attachment #498485 - Flags: review?(dbaron) → review+
Whiteboard: [needs review dbaron] → [needs new patch]
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.
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?
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.
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.  :(
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!
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.
(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!
Do you want to reassign to me, then?
Whiteboard: [needs new patch] → [needs new patch][softblocker]
(In reply to comment #40)
> Do you want to reassign to me, then?

Sure.  Thanks!
Assignee: ehsan → bzbarsky
Attachment #498050 - Attachment is obsolete: true
Attached patch Fix the XSLT issues (obsolete) — Splinter Review
Attachment #505666 - Flags: review?(dbaron)
Whiteboard: [needs new patch][softblocker] → [need review][softblocker]
(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.
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 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
Attachment #505666 - Flags: review?(dbaron) → review+
> 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|....
Attachment #505666 - Attachment is obsolete: true
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
Assignee: bzbarsky → ehsan
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [need review][softblocker] → [softblocker]
Target Milestone: --- → mozilla2.0b10
Depends on: 629172
Depends on: 631359
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
(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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: [softblocker] → [softblocker][fx4-fixed-bugday]
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!
Status: REOPENED → RESOLVED
Closed: 13 years ago13 years ago
Resolution: --- → FIXED
(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.
(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
Blocks: 436635
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: