From http://dev.w3.org/html5/spec/Overview.html#the-bdi-element: The bdi element represents a span of text that is to be isolated from its surroundings for the purposes of bidirectional text formatting. The dir global attribute defaults to auto on this element (it never inherits from the parent element like with other elements). For the purposes of applying the bidirectional algorithm to the contents of a bdi element, user agents must treat the element as a paragraph-level container. For the purposes of applying the bidirectional algorithm to the paragraph-level container that a bdi element finds itself within, the bdi element must be treated like a U+FFFC OBJECT REPLACEMENT CHARACTER (in the same manner that an image or other inline object is handled). The requirements on handling the bdi element for the bidirectional algorithm may be implemented indirectly through the style layer. For example, an HTML+CSS user agent should implement these requirements by implementing the CSS 'unicode-bidi' property. This element is especially useful when embedding user-generated content with an unknown directionality. In this example, usernames are shown along with the number of posts that the user has submitted. If the bdi element were not used, the username of the Arabic user would end up confusing the text (the bidirectional algorithm would put the colon and the number "3" next to the word "User" rather than next to the word "posts"). <ul> <li>User <bdi>jcranmer</bdi>: 12 posts. <li>User <bdi>hober</bdi>: 5 posts. <li>User <bdi>إيان</bdi>: 3 posts. </ul>

This applies on top of the patch in bug 624798.

Comment on attachment 525116 [details] [diff] [review] Implement bidi isolation in layout + } else + TraverseFrames(aBlockFrame, kid, aBpd); {} + mParaLevel = isRTL ? NextOddLevel (mParaLevel) : NextEvenLevel(mParaLevel); NextOddLevel(

Comment on attachment 525114 [details] [diff] [review] Style system support for unicode-bidi: -moz-isolate and -moz-plaintext The following is a review I wrote on an airplane; I haven't get gone through the stuff I can look at now that I'm online: I'd suggest not naming the NS_STYLE_UNICODE_BIDI_* constants with _MOZ_ in the names; without the _MOZ_ is probably better. In CSSParserImpl::ParseUnicodeBidi could you add some comments indicating that the current parsing code is specific to 'unicode-bidi' allowing at most two values? I'm going to avoid asking what 'unicode-bidi: embed bidi-override' means. But whatever it means, I'm a little concerned about implementing it unprefixed, which this seems to be doing. Should we really be doing that? (That said, nsBidiPresUtils::TraverseFrames doesn't exactly seem to be handling it... bidi-override just wins.) Maybe we should instead just avoid accepting both of these values together? Let me see... as far as I can tell, the sensible combinations seem to me to be: normal embed bidi-override isolate isolate bidi-override plaintext and/or plaintext embed (or do they differ some way?) plaintext isolate whereas this patch allows: normal embed isolate bidi-override plaintext (1) embed bidi-override (2) embed plaintext (1) isolate bidi-override isolate plaintext which means my conceptual understanding of what these do is missing two gaps: (1) how 'plaintext' and 'embed plaintext' differ (2) why 'embed bidi-override' is allowed Other than that, this patch looks fine, though I think you should add the complete set of legal combinations (including alternate orders) to other_values in property_database.js.

(In reply to comment #5) > I'm going to avoid asking what 'unicode-bidi: embed bidi-override' > means. But whatever it means, I'm a little concerned about > implementing it unprefixed, which this seems to be doing. Should we > really be doing that? > > (That said, nsBidiPresUtils::TraverseFrames doesn't exactly seem to > be handling it... bidi-override just wins.) > > Maybe we should instead just avoid accepting both of these values > together? I wasn't sure about this either. AFAIC the draft spec at http://dev.w3.org/csswg/css3-writing-modes/#unicode-bidi0 allows both values together: | Value: normal | [ [ embed | isolate ] || [ plaintext | bidi-override ] ] but since the combination doesn't seem to make sense, I deliberately made bidi-override win in practice. I meant to add a comment about this either to the patch or the bug or both, but apparently I forgot to do so. Fantasai, any comments? Should I post to www-style about this?

Posting to www-style would be a good idea.

Comment on attachment 525114 [details] [diff] [review] Style system support for unicode-bidi: -moz-isolate and -moz-plaintext So I think I'd like to see a revised patch here with a slightly more restricted value set (plus my other comments above) -- which also has the advantage of not introducing new completely-unprefixed options.

In the latest draft of http://dev.w3.org/csswg/css3-writing-modes/ the grammar of unicode-bidi has been changed to normal | embed | [ isolate || [ plaintext | bidi-override ] ]

Ok, dbaron and I discussed this yesterday and we came up with some changes that seemed to make sense: - have plaintext imply isolate - have plaintext affect inline elements (since the isolate behavior makes it, bidiwise, effectively the containing block for a bunch of takes) I've edited this into http://dev.w3.org/csswg/css3-writing-modes/#unicode-bidi Does that all make sense to you?

That all sounds reasonable prima facie, though I reserve the right to change my mind when I get to fully implementing plaintext, which will be in another bug ;-) (the "Style system" patch here includes the constants and parsing for plaintext because I didn't want to change the parsing for isolate and then change it again for plaintext). I'm also not sure what "a bunch of takes" means. Is "takes" a typo for "text"?

wrt s/takes/text/, um, yes. :) wrt parsing 'plaintext' before implementing it, we shouldn't do that. http://www.w3.org/TR/CSS/#partial Other than that, the patch looks okay. (Notes: I didn't review the parser bits too closely, and property_database.js looks rather verbose from here.)

(In reply to comment #13) > wrt parsing 'plaintext' before implementing it, we shouldn't do that. > http://www.w3.org/TR/CSS/#partial OK, so I won't check this in until plaintext is done.

Comment on attachment 537430 [details] [diff] [review] Style system patch updated to the latest draft of http://dev.w3.org/csswg/css3-writing-modes/ nsCSSParser.cpp: >+ // look for more keywords >+ nsCSSValue keyword; >+ if (ParseEnum(keyword, nsCSSProps::kUnicodeBidiKTable)) { >+ intValue |= keyword.GetIntValue(); Could you call this |keyword| variable |second| instead since keyword has a different meaning (nsCSSKeywords). nsCSSValue.cpp *and* nsComputedDOMStyle.cpp: Could you PR_STATIC_ASSERT around the added code that NS_STYLE_UNICODE_BIDI_NORMAL is 0? property_database.js: May as well also test the remaining invalid values: embed bidi-override embed -moz-plaintext -moz-isolate normal bidi-override normal bidi-override embed -moz-plaintext normal -moz-plaintext embed -moz-plaintext bidi-override since then you'll have all the pairs tested. :-) nsSVGGlyphFrame: >+ PRBool bidiOverride = (mParent->GetStyleTextReset()->mUnicodeBidi & > NS_STYLE_UNICODE_BIDI_OVERRIDE); Use !! outside the (). r=dbaron with that, though this shouldn't land until you implement both the isolate and plaintext behaviors. Sorry for the delay getting to this. In the future, please include a commit message in patches for review so that I can review the commit message as well.

Rebased to changes from bug 482909

Layout patch rebased and updated with a fix and test for a bug I found while dogfooding it

Comment on attachment 555776 [details] [diff] [review] Implement bidi isolation in layout Review of attachment 555776 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/nsBidiPresUtils.cpp @@ +871,5 @@ > nsBidiPresUtils::TraverseFrames(nsBlockFrame* aBlockFrame, > nsBlockInFlowLineIterator* aLineIter, > nsIFrame* aCurrentFrame, > + BidiParagraphData* aBpd, > + BidiParagraphData* containingParagraph) aContainingParagraph @@ +1078,5 @@ > + if (text->mUnicodeBidi & NS_STYLE_UNICODE_BIDI_ISOLATE) { > + // css "unicode-bidi: isolate" and html5 bdi: > + // resolve the element as a separate paragraph > + TraverseFrames(aBlockFrame, aLineIter, kid, > + aBpd->GetSubParagraph(), aBpd); Why do you need to keep reusing the same sub-paragraph here? Shouldn't you make a new sub-paragraph every time we reach here?

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #18) > Comment on attachment 555776 [details] [diff] [review] > Implement bidi isolation in layout > > + BidiParagraphData* containingParagraph) > > aContainingParagraph Fixed > Why do you need to keep reusing the same sub-paragraph here? Shouldn't you > make a new sub-paragraph every time we reach here? No, because we can reach here multiple times if the sub-paragraph has continuation frames already, for example on non-initial reflow as in reftests/bidi/613149-2b.html

OK, what forces a new subparagraph to start when you find another "isolate" element in the same outer paragraph?

+ if (aContainingParagraph && isFirstFrame) { + aBpd->Reset(aCurrentFrame, aContainingParagraph); + }

While I'm here, I noticed that the aFrame argument to AddUnicharFrame is unnecessary.

Comment on attachment 555962 [details] [diff] [review] Some extra cleanup Review of attachment 555962 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/nsBidiPresUtils.cpp @@ +271,5 @@ > void AppendUnichar(PRUnichar aCh){ mBuffer.Append(aCh); } > > void AppendString(const nsDependentSubstring& aString){ mBuffer.Append(aString); } > > + void AppendControlCharFrame(PRUnichar aCh) I tihnk just "AppendControlChar".

Comment on attachment 555742 [details] [diff] [review] Support for <bdi> in content ># HG changeset patch ># User Simon Montagu <smontagu@smontagu.org> ># Date 1314248919 -10800 ># Node ID d29252fedc5a37164b00f8023ead2773774efd35 ># Parent 9fb645d4f6403c0ed23cc02a62ce5d0e76c08ba1 >Support for HTML5 <element> in content. Bug 613149 I think you want to fix that before landing.

Fixed in my local queue, thanks!

https://hg.mozilla.org/integration/mozilla-inbound/rev/6f03f6a821c0 https://hg.mozilla.org/integration/mozilla-inbound/rev/69581e46f21c https://hg.mozilla.org/integration/mozilla-inbound/rev/09e62ad4229f https://hg.mozilla.org/integration/mozilla-inbound/rev/5d100f15d41f

https://hg.mozilla.org/mozilla-central/rev/6f03f6a821c0 https://hg.mozilla.org/mozilla-central/rev/69581e46f21c https://hg.mozilla.org/mozilla-central/rev/09e62ad4229f https://hg.mozilla.org/mozilla-central/rev/5d100f15d41f

I've created: https://developer.mozilla.org/en/HTML/Element/bdi and updated: https://developer.mozilla.org/en/CSS/unicode-bidi https://developer.mozilla.org/en/Firefox_10_for_developers