Separate specified and effective writing-mode [chromium/src : master]

0 views
Skip to first unread message

Xianzhu Wang (Gerrit)

unread,
Jun 24, 2017, 5:57:38 PM6/24/17
to apavlo...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, dglazko...@chromium.org, Alexis Menard, chromium...@chromium.org, Rob Buis

Xianzhu Wang posted comments on this change.

View Change

Patch set 2:Commit-Queue +1

    To view, visit change 547368. To unsubscribe, visit settings.

    Gerrit-Project: chromium/src
    Gerrit-Branch: master
    Gerrit-MessageType: comment
    Gerrit-Change-Id: I878de77af4ff201475a3158a9877b99c9ce44eac
    Gerrit-Change-Number: 547368
    Gerrit-PatchSet: 2
    Gerrit-Owner: Xianzhu Wang <wangx...@chromium.org>
    Gerrit-Reviewer: Xianzhu Wang <wangx...@chromium.org>
    Gerrit-CC: Alexis Menard <alexis...@intel.com>
    Gerrit-CC: Rob Buis <rob....@samsung.com>
    Gerrit-Comment-Date: Sat, 24 Jun 2017 21:57:36 +0000
    Gerrit-HasComments: No
    Gerrit-HasLabels: Yes

    Xianzhu Wang (Gerrit)

    unread,
    Jun 25, 2017, 12:28:50 AM6/25/17
    to apavlo...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, dglazko...@chromium.org, Commit Bot, Alexis Menard, chromium...@chromium.org, Rob Buis

    Xianzhu Wang posted comments on this change.

    View Change

    Patch set 3:Commit-Queue +1

      To view, visit change 547368. To unsubscribe, visit settings.

      Gerrit-Project: chromium/src
      Gerrit-Branch: master
      Gerrit-MessageType: comment
      Gerrit-Change-Id: I878de77af4ff201475a3158a9877b99c9ce44eac
      Gerrit-Change-Number: 547368
      Gerrit-PatchSet: 3
      Gerrit-Owner: Xianzhu Wang <wangx...@chromium.org>
      Gerrit-Reviewer: Xianzhu Wang <wangx...@chromium.org>
      Gerrit-CC: Alexis Menard <alexis...@intel.com>
      Gerrit-CC: Commit Bot <commi...@chromium.org>
      Gerrit-CC: Rob Buis <rob....@samsung.com>
      Gerrit-Comment-Date: Sun, 25 Jun 2017 04:28:47 +0000
      Gerrit-HasComments: No
      Gerrit-HasLabels: Yes

      Xianzhu Wang (Gerrit)

      unread,
      Jun 25, 2017, 1:41:58 PM6/25/17
      to apavlo...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, dglazko...@chromium.org, chromium...@chromium.org, Rob Buis, Commit Bot, Alexis Menard

      Xianzhu Wang uploaded patch set #4 to this change.

      View Change

      Separate specified and effective writing-mode

      Some table components need to use the table's writing-mode, while
      still need the specified writing-mode for getComputedStyle() API and
      inheritance.

      BUG=736093

      Change-Id: I878de77af4ff201475a3158a9877b99c9ce44eac
      ---
      A third_party/WebKit/LayoutTests/fast/css/getComputedStyle/getComputedStyle-writing-mode-table.html
      M third_party/WebKit/Source/core/css/CSSProperties.json5
      M third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp
      M third_party/WebKit/Source/core/css/ComputedStyleExtraFields.json5
      M third_party/WebKit/Source/core/css/resolver/MatchedPropertiesCache.cpp
      M third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp
      M third_party/WebKit/Source/core/css/resolver/StyleResolverState.h
      M third_party/WebKit/Source/core/style/ComputedStyle.h
      8 files changed, 96 insertions(+), 6 deletions(-)

      To view, visit change 547368. To unsubscribe, visit settings.

      Gerrit-Project: chromium/src
      Gerrit-Branch: master
      Gerrit-MessageType: newpatchset
      Gerrit-Change-Id: I878de77af4ff201475a3158a9877b99c9ce44eac
      Gerrit-Change-Number: 547368
      Gerrit-PatchSet: 4

      Xianzhu Wang (Gerrit)

      unread,
      Jun 25, 2017, 1:44:05 PM6/25/17
      to nainar, apavlo...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, dglazko...@chromium.org

      Xianzhu Wang would like nainar to review this change.

      View Change

      Separate specified and effective writing-mode

      Some table components need to use the table's writing-mode, while
      still need the specified writing-mode for getComputedStyle() API and
      inheritance.

      BUG=736093

      Change-Id: I878de77af4ff201475a3158a9877b99c9ce44eac
      ---
      A third_party/WebKit/LayoutTests/fast/css/getComputedStyle/getComputedStyle-writing-mode-table.html
      M third_party/WebKit/Source/core/css/CSSProperties.json5
      M third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp
      M third_party/WebKit/Source/core/css/ComputedStyleExtraFields.json5
      M third_party/WebKit/Source/core/css/resolver/MatchedPropertiesCache.cpp
      M third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp
      M third_party/WebKit/Source/core/css/resolver/StyleResolverState.h
      M third_party/WebKit/Source/core/style/ComputedStyle.h
      8 files changed, 96 insertions(+), 6 deletions(-)


      To view, visit change 547368. To unsubscribe, visit settings.

      Gerrit-Project: chromium/src
      Gerrit-Branch: master
      Gerrit-MessageType: newchange
      Gerrit-Change-Id: I878de77af4ff201475a3158a9877b99c9ce44eac
      Gerrit-Change-Number: 547368
      Gerrit-PatchSet: 4
      Gerrit-Owner: Xianzhu Wang <wangx...@chromium.org>
      Gerrit-Reviewer: Xianzhu Wang <wangx...@chromium.org>
      Gerrit-Reviewer: nainar <nai...@chromium.org>

      Rune Lillesveen (Gerrit)

      unread,
      Jun 25, 2017, 2:07:49 PM6/25/17
      to Xianzhu Wang, apavlo...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, dglazko...@chromium.org, Rune Lillesveen, nainar, Commit Bot, Alexis Menard, chromium...@chromium.org, Rob Buis

      Rune Lillesveen posted comments on this change.

      View Change

      Patch set 4:

      To what extent do we need to store the used value for table display elements in ComputedStyle?

      Since you're hiding the used (effective) value in GetWritingMode(), it's not obvious from this patch where this matters, but could this be handled by logic in the table layout code instead? Would that be expensive/much code?

      I think I would've used the CSS terms computed/used instead of specified/effective.

        To view, visit change 547368. To unsubscribe, visit settings.

        Gerrit-Project: chromium/src
        Gerrit-Branch: master
        Gerrit-MessageType: comment
        Gerrit-Change-Id: I878de77af4ff201475a3158a9877b99c9ce44eac
        Gerrit-Change-Number: 547368
        Gerrit-PatchSet: 4
        Gerrit-Owner: Xianzhu Wang <wangx...@chromium.org>
        Gerrit-Reviewer: Rune Lillesveen <ru...@opera.com>
        Gerrit-Reviewer: Xianzhu Wang <wangx...@chromium.org>
        Gerrit-Reviewer: nainar <nai...@chromium.org>
        Gerrit-CC: Alexis Menard <alexis...@intel.com>
        Gerrit-CC: Commit Bot <commi...@chromium.org>
        Gerrit-CC: Rob Buis <rob....@samsung.com>
        Gerrit-Comment-Date: Sun, 25 Jun 2017 18:07:45 +0000
        Gerrit-HasComments: No
        Gerrit-HasLabels: No

        Xianzhu Wang (Gerrit)

        unread,
        Jun 25, 2017, 3:40:23 PM6/25/17
        to apavlo...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, dglazko...@chromium.org, Koji Ishii, Rune Lillesveen, nainar, Commit Bot, Alexis Menard, chromium...@chromium.org, Rob Buis

        Xianzhu Wang posted comments on this change.

        View Change

        Patch set 4:

        +kojii

        Patch Set 4:

        To what extent do we need to store the used value for table display elements in ComputedStyle?

        Since you're hiding the used (effective) value in GetWritingMode(), it's not obvious from this patch where this matters, but could this be handled by logic in the table layout code instead? Would that be expensive/much code?

        I think I would've used the CSS terms computed/used instead of specified/effective.

        Your questions let me think of how we should categorize the "effective" values of 'direction' and 'writing-mode' of the table components. It seems that they may be something between computed values and used values, depending on how we define their effects on computed values of other properties depending on them.

        For example,
        <table>
        <tr id="tr" style="writing-mode: vertical-rl">
        ...
        </tr>
        </table>
        What should "getComputedStyle(tr).inlineSize" evaluate?

        I'm not sure what the best definition of the above behavior is, but it seems most convenient to let these properties depend on the "effective"[1] values of 'direction' and 'writing-mode' instead of their specified/computed values, otherwise we would need another whole set of functions (besides the many current direction-aware ComputedStyle methods) to convert the results from specified/computed direction/writing-mode to used direction/writing-mode.

        [1] I still use "effective" because under this definition, it affects computed values of other properties so it's not just "used". It's also not "computed" because according to the definition of computed style, the computed value should be the specified value. Perhaps we should define a special computed value rule for writing-mode/direction of the table components to use the "effective" values as their computed values?

          To view, visit change 547368. To unsubscribe, visit settings.

          Gerrit-Project: chromium/src
          Gerrit-Branch: master
          Gerrit-MessageType: comment
          Gerrit-Change-Id: I878de77af4ff201475a3158a9877b99c9ce44eac
          Gerrit-Change-Number: 547368
          Gerrit-PatchSet: 4
          Gerrit-Owner: Xianzhu Wang <wangx...@chromium.org>
          Gerrit-Reviewer: Koji Ishii <ko...@chromium.org>
          Gerrit-Reviewer: Rune Lillesveen <ru...@opera.com>
          Gerrit-Reviewer: Xianzhu Wang <wangx...@chromium.org>
          Gerrit-Reviewer: nainar <nai...@chromium.org>
          Gerrit-CC: Alexis Menard <alexis...@intel.com>
          Gerrit-CC: Commit Bot <commi...@chromium.org>
          Gerrit-CC: Rob Buis <rob....@samsung.com>
          Gerrit-Comment-Date: Sun, 25 Jun 2017 19:40:17 +0000
          Gerrit-HasComments: No
          Gerrit-HasLabels: No

          Xianzhu Wang (Gerrit)

          unread,
          Jun 25, 2017, 6:22:10 PM6/25/17
          to apavlo...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, dglazko...@chromium.org, Koji Ishii, Rune Lillesveen, nainar, Commit Bot, Alexis Menard, chromium...@chromium.org, Rob Buis

          Xianzhu Wang posted comments on this change.

          View Change

          Patch set 4:

          Please hold off on reviewing this CL.

          This implementation may cause trouble of future implementation of logical properties (https://drafts.csswg.org/css-logical). I've sent an email to www-style (https://lists.w3.org/Archives/Public/www-style/2017Jun/0030.html) for clarification.

            To view, visit change 547368. To unsubscribe, visit settings.

            Gerrit-Project: chromium/src
            Gerrit-Branch: master
            Gerrit-MessageType: comment
            Gerrit-Change-Id: I878de77af4ff201475a3158a9877b99c9ce44eac
            Gerrit-Change-Number: 547368
            Gerrit-PatchSet: 4
            Gerrit-Owner: Xianzhu Wang <wangx...@chromium.org>
            Gerrit-Reviewer: Koji Ishii <ko...@chromium.org>
            Gerrit-Reviewer: Rune Lillesveen <ru...@opera.com>
            Gerrit-Reviewer: Xianzhu Wang <wangx...@chromium.org>
            Gerrit-Reviewer: nainar <nai...@chromium.org>
            Gerrit-CC: Alexis Menard <alexis...@intel.com>
            Gerrit-CC: Commit Bot <commi...@chromium.org>
            Gerrit-CC: Rob Buis <rob....@samsung.com>
            Gerrit-Comment-Date: Sun, 25 Jun 2017 22:22:05 +0000
            Gerrit-HasComments: No
            Gerrit-HasLabels: No

            nainar (Gerrit)

            unread,
            Jun 28, 2017, 4:03:51 AM6/28/17
            to Xianzhu Wang, apavlo...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, dglazko...@chromium.org, Koji Ishii, Rune Lillesveen, Commit Bot, Alexis Menard, chromium...@chromium.org, Rob Buis

            nainar posted comments on this change.

            View Change

            Patch set 4:

            Taking myself off so it doesnt show in my incoming stack. Please add me back when ready for review

              To view, visit change 547368. To unsubscribe, visit settings.

              Gerrit-Project: chromium/src
              Gerrit-Branch: master
              Gerrit-MessageType: comment
              Gerrit-Change-Id: I878de77af4ff201475a3158a9877b99c9ce44eac
              Gerrit-Change-Number: 547368
              Gerrit-PatchSet: 4
              Gerrit-Owner: Xianzhu Wang <wangx...@chromium.org>
              Gerrit-Reviewer: Koji Ishii <ko...@chromium.org>
              Gerrit-Reviewer: Rune Lillesveen <ru...@opera.com>
              Gerrit-Reviewer: Xianzhu Wang <wangx...@chromium.org>
              Gerrit-Reviewer: nainar <nai...@chromium.org>
              Gerrit-CC: Alexis Menard <alexis...@intel.com>
              Gerrit-CC: Commit Bot <commi...@chromium.org>
              Gerrit-CC: Rob Buis <rob....@samsung.com>
              Gerrit-Comment-Date: Wed, 28 Jun 2017 08:03:47 +0000
              Gerrit-HasComments: No
              Gerrit-HasLabels: No

              Xianzhu Wang (Gerrit)

              unread,
              Jun 28, 2017, 12:52:09 PM6/28/17
              to apavlo...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, dglazko...@chromium.org, Koji Ishii, Rune Lillesveen, Commit Bot, Alexis Menard, chromium...@chromium.org, Rob Buis

              Xianzhu Wang abandoned this change.

              View Change

              Abandoned The approach seems incorrect according to Firefox's behavior described in https://lists.w3.org/Archives/Public/www-style/2017Jun/0030.html. I have fixed crbug.com/727173 with https://chromium-review.googlesource.com/c/547740/. crbug.com/736093 is much less urgent to fix. Let's investigate more. Sorry for the noise.

              To view, visit change 547368. To unsubscribe, visit settings.

              Gerrit-Project: chromium/src
              Gerrit-Branch: master
              Gerrit-MessageType: abandon
              Gerrit-Change-Id: I878de77af4ff201475a3158a9877b99c9ce44eac
              Gerrit-Change-Number: 547368
              Gerrit-PatchSet: 4
              Gerrit-Owner: Xianzhu Wang <wangx...@chromium.org>
              Gerrit-Reviewer: Koji Ishii <ko...@chromium.org>
              Gerrit-Reviewer: Rune Lillesveen <ru...@opera.com>
              Gerrit-Reviewer: Xianzhu Wang <wangx...@chromium.org>
              Reply all
              Reply to author
              Forward
              0 new messages