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

RESOLVED FIXED in mozilla10

Status

()

Core
Layout: Text
RESOLVED FIXED
7 years ago
4 years ago

People

(Reporter: smontagu, Assigned: smontagu)

Tracking

(Blocks: 2 bugs, {dev-doc-complete, html5, rtl})

Trunk
mozilla10
dev-doc-complete, html5, rtl
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(4 attachments, 5 obsolete attachments)

(Assignee)

Description

7 years ago
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>
(Assignee)

Updated

7 years ago
(Assignee)

Updated

7 years ago
Blocks: 613154

Updated

6 years ago
Version: unspecified → Trunk
(Assignee)

Comment 1

6 years ago
Created attachment 525110 [details] [diff] [review]
Support for <bdi> in content
Assignee: nobody → smontagu
Attachment #525110 - Flags: review?(jst)
(Assignee)

Comment 2

6 years ago
Created attachment 525114 [details] [diff] [review]
Style system support for unicode-bidi: -moz-isolate and -moz-plaintext
Attachment #525114 - Flags: review?(dbaron)
(Assignee)

Comment 3

6 years ago
Created attachment 525116 [details] [diff] [review]
Implement bidi isolation in layout

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.
(Assignee)

Comment 6

6 years ago
(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
(Assignee)

Updated

6 years ago
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-
(Assignee)

Comment 9

6 years ago
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 ] ]
Attachment #525114 - Attachment is obsolete: true

Comment 10

6 years ago
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?
(Assignee)

Comment 11

6 years ago
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"?
(Assignee)

Comment 12

6 years ago
Created attachment 537430 [details] [diff] [review]
Style system patch updated to the latest draft of http://dev.w3.org/csswg/css3-writing-modes/
Attachment #536698 - Attachment is obsolete: true
Attachment #537430 - Flags: review?(dbaron)
Attachment #537430 - Flags: feedback?(fantasai.bugs)

Comment 13

6 years ago
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.)
(Assignee)

Comment 14

6 years ago
(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

Updated

6 years ago
Blocks: 568516
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+
(Assignee)

Comment 16

6 years ago
Created attachment 555742 [details] [diff] [review]
Support for <bdi> in content

Rebased to changes from bug 482909
Attachment #525110 - Attachment is obsolete: true
Attachment #555742 - Flags: review?(peterv)
Attachment #525110 - Flags: review?(jst)
(Assignee)

Comment 17

6 years ago
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
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?
(Assignee)

Comment 19

6 years ago
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
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?
(Assignee)

Comment 21

6 years ago
+    if (aContainingParagraph && isFirstFrame) {
+      aBpd->Reset(aCurrentFrame, aContainingParagraph);
+    }
Attachment #555958 - Flags: review?(roc) → review+
(Assignee)

Comment 22

6 years ago
Created attachment 555962 [details] [diff] [review]
Some extra cleanup

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+
(Assignee)

Updated

6 years ago
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.
(Assignee)

Comment 25

6 years ago
Fixed in my local queue, thanks!
(Assignee)

Comment 26

6 years ago
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
Flags: in-testsuite+
Target Milestone: mozilla9 → mozilla10
Keywords: dev-doc-needed
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
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED

Updated

6 years ago
Depends on: 695861
(Assignee)

Updated

6 years ago
Depends on: 698706

Updated

6 years ago
Depends on: 701504
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
Keywords: dev-doc-needed → dev-doc-complete
(Assignee)

Updated

6 years ago
Depends on: 707098

Updated

6 years ago
Attachment #537430 - Flags: feedback?(fantasai.bugs)

Updated

4 years ago
Depends on: 859093
You need to log in before you can comment on or make changes to this bug.