Last Comment Bug 613149 - Support HTML5 bdi element and CSS property unicode-bidi: isolate
: Support HTML5 bdi element and CSS property unicode-bidi: isolate
Status: RESOLVED FIXED
: dev-doc-complete, html5, rtl
Product: Core
Classification: Components
Component: Layout: Text (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: mozilla10
Assigned To: Simon Montagu :smontagu
:
:
Mentors:
http://dev.w3.org/html5/spec/Overview...
Depends on: 695861 698706 701504 707098 859093
Blocks: html5 html5bidi 662288 676245
  Show dependency treegraph
 
Reported: 2010-11-18 01:13 PST by Simon Montagu :smontagu
Modified: 2013-04-08 04:38 PDT (History)
16 users (show)
smontagu: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Support for <bdi> in content (1.23 KB, patch)
2011-04-11 11:21 PDT, Simon Montagu :smontagu
no flags Details | Diff | Splinter Review
Style system support for unicode-bidi: -moz-isolate and -moz-plaintext (15.64 KB, patch)
2011-04-11 11:27 PDT, Simon Montagu :smontagu
dbaron: review-
Details | Diff | Splinter Review
Implement bidi isolation in layout (8.82 KB, patch)
2011-04-11 11:28 PDT, Simon Montagu :smontagu
roc: review+
Details | Diff | Splinter Review
Style system patch updated to comments (15.82 KB, patch)
2011-06-01 12:31 PDT, Simon Montagu :smontagu
no flags Details | Diff | Splinter Review
Style system patch updated to the latest draft of http://dev.w3.org/csswg/css3-writing-modes/ (15.67 KB, patch)
2011-06-05 01:47 PDT, Simon Montagu :smontagu
dbaron: review+
Details | Diff | Splinter Review
Support for <bdi> in content (1.47 KB, patch)
2011-08-25 08:29 PDT, Simon Montagu :smontagu
peterv: review+
Details | Diff | Splinter Review
Implement bidi isolation in layout (17.84 KB, patch)
2011-08-25 10:24 PDT, Simon Montagu :smontagu
no flags Details | Diff | Splinter Review
Implement bidi isolation in layout (17.85 KB, patch)
2011-08-25 22:07 PDT, Simon Montagu :smontagu
roc: review+
Details | Diff | Splinter Review
Some extra cleanup (2.47 KB, patch)
2011-08-25 23:33 PDT, Simon Montagu :smontagu
roc: review+
Details | Diff | Splinter Review

Description Simon Montagu :smontagu 2010-11-18 01:13:36 PST
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>
Comment 1 Simon Montagu :smontagu 2011-04-11 11:21:44 PDT
Created attachment 525110 [details] [diff] [review]
Support for <bdi> in content
Comment 2 Simon Montagu :smontagu 2011-04-11 11:27:05 PDT
Created attachment 525114 [details] [diff] [review]
Style system support for unicode-bidi: -moz-isolate and -moz-plaintext
Comment 3 Simon Montagu :smontagu 2011-04-11 11:28:42 PDT
Created attachment 525116 [details] [diff] [review]
Implement bidi isolation in layout

This applies on top of the patch in bug 624798.
Comment 4 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-04-11 13:16:03 PDT
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 5 David Baron :dbaron: ⌚️UTC-10 2011-05-11 01:21:55 PDT
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.
Comment 6 Simon Montagu :smontagu 2011-05-11 01:59:18 PDT
(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?
Comment 7 David Baron :dbaron: ⌚️UTC-10 2011-05-19 08:02:46 PDT
Posting to www-style would be a good idea.
Comment 8 David Baron :dbaron: ⌚️UTC-10 2011-05-27 14:48:18 PDT
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.
Comment 9 Simon Montagu :smontagu 2011-06-01 12:31:01 PDT
Created attachment 536698 [details] [diff] [review]
Style system patch updated to comments

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 ] ]
Comment 10 fantasai 2011-06-02 17:52:17 PDT
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?
Comment 11 Simon Montagu :smontagu 2011-06-05 01:16:31 PDT
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"?
Comment 12 Simon Montagu :smontagu 2011-06-05 01:47:57 PDT
Created attachment 537430 [details] [diff] [review]
Style system patch updated to the latest draft of http://dev.w3.org/csswg/css3-writing-modes/
Comment 13 fantasai 2011-06-05 23:21:36 PDT
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.)
Comment 14 Simon Montagu :smontagu 2011-06-06 09:05:24 PDT
(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 15 David Baron :dbaron: ⌚️UTC-10 2011-08-16 15:39:05 PDT
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.
Comment 16 Simon Montagu :smontagu 2011-08-25 08:29:11 PDT
Created attachment 555742 [details] [diff] [review]
Support for <bdi> in content

Rebased to changes from bug 482909
Comment 17 Simon Montagu :smontagu 2011-08-25 10:24:37 PDT
Created attachment 555776 [details] [diff] [review]
Implement bidi isolation in layout

Layout patch rebased and updated with a fix and test for a bug I found while dogfooding it
Comment 18 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-08-25 18:59:10 PDT
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?
Comment 19 Simon Montagu :smontagu 2011-08-25 22:07:49 PDT
Created attachment 555958 [details] [diff] [review]
Implement bidi isolation in layout

(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
Comment 20 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-08-25 22:28:37 PDT
OK, what forces a new subparagraph to start when you find another "isolate" element in the same outer paragraph?
Comment 21 Simon Montagu :smontagu 2011-08-25 22:53:23 PDT
+    if (aContainingParagraph && isFirstFrame) {
+      aBpd->Reset(aCurrentFrame, aContainingParagraph);
+    }
Comment 22 Simon Montagu :smontagu 2011-08-25 23:33:42 PDT
Created attachment 555962 [details] [diff] [review]
Some extra cleanup

While I'm here, I noticed that the aFrame argument to AddUnicharFrame is unnecessary.
Comment 23 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-08-26 05:08:42 PDT
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 24 :Ms2ger (⌚ UTC+1/+2) 2011-09-04 04:40:26 PDT
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.
Comment 25 Simon Montagu :smontagu 2011-09-04 04:47:57 PDT
Fixed in my local queue, thanks!

Note You need to log in before you can comment on or make changes to this bug.