Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: disable attributes copy to parent #157

Merged
merged 8 commits into from
Oct 16, 2022

Conversation

akabekobeko
Copy link
Member

@akabekobeko akabekobeko commented Oct 9, 2022

refs #151

  • 見出し属性のセクションへのコピーを無効化
  • コピーで hidden 属性を独自に処理していたが、これは維持
    • これをなくすと # Heading {hidden} が処理されない
    • {} 記法の仕様かバグ対策と思われるので、互換性のため踏襲
  • section の深さにあわせて aria-labelledby 属性へ heading-N を設定するようにした
  • ドキュメント
    • セクション処理における属性コピーの説明と CSS サンプルを削除
    • aria-labelledby の説明を追記

@akabekobeko akabekobeko self-assigned this Oct 9, 2022
@akabekobeko akabekobeko added this to the v2.0.0 milestone Oct 9, 2022

section.title > h1:first-child {
}

.level1 {
}
.level2 {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

属性コピーを無効化したので関連する記述を削除しました。

@@ -80,7 +42,12 @@ const sectionize = (node: any, ancestors: Parent[]) => {
endIndex > 0 ? endIndex : undefined,
);

const hProperties = checkProperties(node, depth);
const hProperties = { class: [`level${depth}`] };
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

階層の深さ class は残します。

Comment on lines +47 to +50
// {hidden} specifier
if (Object.keys(node.data.hProperties).includes('hidden')) {
node.data.hProperties.hidden = 'hidden';
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

checkProperties の hidden 属性処理を移植しました。{} 記法で {hidden} としてもこの属性は出力されないようです。この処理はその対策と思われるので checkProperties 削除後も互換のために維持しておきます。

});
const expected = `<section class="level1"><h1 id="foo">Heading <em>test</em></h1></section>`;
expect(received).toBe(expected);
});
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

仕様変更によるテスト修正です。

あわせて utils の利用を廃止しました。utils は整形された MDAST と HTML の両方を判定するのですが以下の理由により利用をやめてゆきます。

  • HTML をテストすれば十分
  • expect がテスト対象と別ファイルにあると失敗時の出力がわかりにくい

@akabekobeko akabekobeko marked this pull request as ready for review October 10, 2022 07:06
const hProperties = checkProperties(node, depth);
const hProperties = {
class: [`level${depth}`],
'aria-labelledby': `heading-${depth}`,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

要望通り、新規に aria-labelledby 属性を追加しました。値は深さに連動します。

@MurakamiShinyu
Copy link
Member

  • コピーで hidden 属性を独自に処理していたが、これは維持
    • これをなくすと # Heading {hidden} が処理されない
    • {} 記法の仕様かバグ対策と思われるので、互換性のため踏襲

hidden属性などHTMLのブール属性は hidden のように名前だけの指定あるいは hidden="" のように空文字列の値の指定あるいは hidden="hidden" という名前と値が同じ指定によって真の値を、その属性を指定しないことで偽の値を表すことになってます。vfmの属性記法でも同様であるべきだし、また、hiddenに限らずほかのブール属性でも同様であるべきです。

そこで hidden 以外のブール属性として autofocus 属性でテストしてみました。結果は、autofocus だけの指定も autofocus="" の指定も無視されてしまいます。autofocus=autofocus と指定すると autofocus が出力されます。

# Foo {autofocus}
↓
<h1 id="foo">Foo</h1>
# Foo {autofocus=""}
↓
<h1 id="foo">Foo</h1>
# Foo {autofocus=autofocus}
↓
<h1 autofocus id="foo">Foo</h1>

興味深いことに、HTML仕様でブール属性として定義されていない属性や未定義の属性については空の値でもその属性が出力されます。

# Foo {tabindex aaa bbb="" title="title"}
↓
<h1 tabindex="" aaa="" bbb="" title="title" id="foo">Foo</h1>

このテスト結果から、hidden 属性について特別に処理する必要があった理由は分かりました。hidden 属性以外についても同様に処理するとよいのでないかと思います。つまり、{ } 内の属性の記法が名前だけあるいは空の値の指定であるならば、その属性名を出力する。HTML構文では属性名だけなら空の値の指定 ="" が省略されていることになるので今まで bbb=""bbb="" が出力されたのが bbb だけの出力になっても問題ないと思います。

それから、見出し以外での { } 記法による属性は同じ扱いであるべきです。テストしたところ、見出し以外ではhidden属性が無視されてます:

[Foo](#foo){hidden}
↓
<p><a href="#foo">Foo</a></p>

@MurakamiShinyu
Copy link
Member

section の深さにあわせて aria-labelledby 属性へ heading-N を設定するようにした

aria-labelledby 属性はラベルとなる要素の id を指定するものです。sectionにとってはその子要素である見出し要素がラベルになるので見出しの id をsectionのaria-labelledby 属性に設定します。

#151 (comment)

このコメントに書いたHTMLの例、

<section class="level1" aria-labelledby="heading-1">
  <h1 class="one" id="heading-1">Heading 1</h1>
  <p>One.</p>
  <section class="level2" aria-labelledby="heading-2">
    <h2 class="two" id="heading-2">Heading 2</h2>
    <p>Two.</p>
  </section>
</section>

この例での aria-labelledby="heading-1"aria-labelledby="heading-2" はそれぞれ見出しの id="heading-1" id="heading-2" を参照するものです。それらは見出しの文字列「Heading 1」「Heading 2」から生成されるidです。「Foo」のような意味のない文字列のほうが例としてよかったですね。

@MurakamiShinyu
Copy link
Member

このPRのタイトルは "feat: disable attributes copy to parent" なので、見出し(→section)だけでなく画像(→figure)についての変更も、このPRに入れるとよいと思います。

![Text](url) の形式の画像をfigure囲みに変換する仕様はPandocに準拠してますが、Pandocでは ![Text](url){attributes} で指定したattributesはimg要素に出力されるだけで、それをfigureにコピーするようにしたのはVFM独自の拡張でした。その属性コピーをやめる変更によって、Pandocと同様になるとよいです。

Pandocでの変換の例:

$ pandoc
![Text](image.png){#aaa .bbb width=250}
↓
<figure>
<img src="image.png" id="aaa" class="bbb" width="250" alt="Text" />
<figcaption aria-hidden="true">Text</figcaption>
</figure>

@akabekobeko
Copy link
Member Author

ひとつの PR として対応する機能が多すぎるので分割したほうがよさそうです。

  • aria-labelledby だけ見出しの id からコピーする
    • 本 PR で対応
  • hidden 属性は互換の意味で本 PR の対応を採用
    • 本質的な問題は他の属性も含めて別 Issue、PR で改めて扱う

とします。平日は時間が取れなさそうなので来週末以降になります。

…ttribute

In addition, the type definitions of KeyValue are commonized.
@akabekobeko
Copy link
Member Author

@MurakamiShinyu
6396aa3 にて見出しの id 属性がある場合は <section>aria-labelledby 属性に値をコピーするようにしました。あわせてテストやドキュメントも最新の状態に更新しています。

レビューをお願いします。

@MurakamiShinyu
Copy link
Member

動作確認しました。よいと思います。

ドキュメントの次のところ:

見出しに id 属性がある場合は、セクションの aria-labelledby 属性に値をコピーします

見出しには id が自動生成されるので常に id 属性があるのでないでしょうか? そうであれば、
「見出しの id 属性の値をセクションの aria-labelledby 属性にコピーします」
という説明のほうがよいと思います。

@akabekobeko
Copy link
Member Author

@MurakamiShinyu
fe85733 にて id コピーの説明を修正しました。現在は見出しがどのような文字列であろうとも明示的に {} で指定しない限り、文字列をそのまま id にしています。そのため「常に」を入れるか迷ったのですが、この挙動が好ましくないという意見もありそうです。

よって、単に見出しの id をコピーするという説明としました。

Copy link
Member

@MurakamiShinyu MurakamiShinyu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 LGTM

@akabekobeko akabekobeko merged commit 3690e17 into main Oct 16, 2022
@akabekobeko akabekobeko deleted the feat/disable-attributes-copy-to-parent branch October 16, 2022 10:17
MurakamiShinyu added a commit to vivliostyle/vivliostyle.js that referenced this pull request Feb 27, 2023
This resolves the problem on VFM v2 change:

- vivliostyle/vfm#157

With the following Markdown,

```md
 # Table of Contents {.toc}

- [One](one.html)
- [Two](two.html)
```

VFM v1 generates the following HTML:

```html
    <section class="level1 toc" id="table-of-contents">
      <h1 class="toc">Table of Contents</h1>
      <ul>
        <li><a href="one.html">One</a></li>
        <li><a href="two.html">Two</a></li>
      </ul>
    </section>
```

Vivliostyle.js recognizes the section as TOC because "toc" class is found in the section element.

This is changed in VFM v2. It generates the following HTML:

```html
    <section class="level1" aria-labelledby="table-of-contents">
      <h1 class="toc" id="table-of-contents">Table of Contents</h1>
      <ul>
        <li><a href="one.html">One</a></li>
        <li><a href="two.html">Two</a></li>
      </ul>
    </section>
```

The "toc" class is not found in the section element, so the TOC detection has to be tweaked using the selector,
`section:has(>:first-child:is(h1,h2,h3,h4,h5,h6):is(.toc,#toc))`.

Note: the stylesheet for TOC that was a part of UserAgentBaseCSS is now a separate stylesheet UserAgentTocCSS.
This change is necessary because uaStylesheetBaseFetcher cannot handle the `:has()` selector.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants