Closed Bug 613149 Opened 14 years ago Closed 13 years ago

Support HTML5 bdi element and CSS property unicode-bidi: isolate

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla10

People

(Reporter: smontagu, Assigned: smontagu)

References

(Blocks 2 open bugs, )

Details

(Keywords: dev-doc-complete, html5, rtl)

Attachments

(4 files, 5 obsolete files)

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>
Blocks: html5bidi
Version: unspecified → Trunk
Attached patch Support for <bdi> in content (obsolete) — Splinter Review
Assignee: nobody → smontagu
Attachment #525110 - Flags: review?(jst)
This applies on top of the patch in bug 624798.
Attachment #525116 - Flags: review?(roc)
Comment on attachment 525116 [details] [diff] [review]
Implement bidi isolation in layout

+      } else 
+        TraverseFrames(aBlockFrame, kid, aBpd);

{}

+    mParaLevel = isRTL ? NextOddLevel (mParaLevel) : NextEvenLevel(mParaLevel);

NextOddLevel(
Attachment #525116 - Flags: review?(roc) → review+
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?
Target Milestone: --- → mozilla5
Version: Trunk → Other Branch
Target Milestone: mozilla5 → ---
Version: Other Branch → Trunk
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.
Attachment #525114 - Flags: review?(dbaron) → review-
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 ] ]
Attachment #525114 - Attachment is obsolete: true
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"?
Attachment #536698 - Attachment is obsolete: true
Attachment #537430 - Flags: review?(dbaron)
Attachment #537430 - Flags: feedback?(fantasai.bugs)
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.
Blocks: 662288
Blocks: html
Blocks: 676245
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.
Attachment #537430 - Flags: review?(dbaron) → review+
Rebased to changes from bug 482909
Attachment #525110 - Attachment is obsolete: true
Attachment #555742 - Flags: review?(peterv)
Attachment #525110 - Flags: review?(jst)
Layout patch rebased and updated with a fix and test for a bug I found while dogfooding it
Attachment #525116 - Attachment is obsolete: true
Attachment #555776 - Flags: review?(roc)
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
Attachment #555776 - Attachment is obsolete: true
Attachment #555958 - Flags: review?(roc)
Attachment #555776 - Flags: review?(roc)
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.
Attachment #555962 - Flags: review?(roc)
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".
Attachment #555962 - Flags: review?(roc) → review+
Attachment #555742 - Flags: review?(peterv) → review+
Target Milestone: --- → mozilla9
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!
Keywords: dev-doc-needed
Depends on: 695861
Depends on: 698706
Depends on: 701504
Depends on: 707098
Attachment #537430 - Flags: feedback?(fantasai.bugs)
Depends on: 859093
Depends on: 1445983
You need to log in before you can comment on or make changes to this bug.