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

サイドバーのバグ潰しへの協力のお願い #476

Closed
saki7 opened this issue Nov 6, 2017 · 63 comments
Closed

サイドバーのバグ潰しへの協力のお願い #476

saki7 opened this issue Nov 6, 2017 · 63 comments

Comments

@saki7
Copy link
Contributor

saki7 commented Nov 6, 2017

文責: @cpprefjp/design
cc: @cpprefjp/workingteam

今日からサイドバーを新しくしました。編集者の皆さんは、バグや怪しい挙動を見つけた際に cpprefjp/kunai まで報告をお願いします。

Issueを立てる際には、 Projects の指定を、このIssueと同じ「Sidebar v2」に指定してください。既知の問題などはプロジェクトページで管理しているので、報告する際の参考にしてください。

問題が起きた時のために、画面左上 Legacy sidebar [x] のチェックを入れると以前の表示に戻るようにしてあります。大きな問題が解消したら、以前のサイドバーへのフォールバックを取り除きます。その時点で完了とします。

Sidebar v1

  • 単純なツリー構造なので、上下方向に無限に長くなることが避けられない
  • 全て静的なHTMLとして生成して埋め込んでいたため、cpprefjp内の全てのページが500KBほどあった
  • 上記の理由によりHTMLをminifyせざるをえず、site_generatorの再変換が4時間以上かかっていた

Sidebar v2

  • ツリー構造をやめた
  • crsearch.json のデータをそのまま利用
  • HTMLに埋め込まないため、ページが500KB→30KB

@cpprefjp/design: Sidebar v2に上がったIssueのトリアージをお願いします。

@saki7 saki7 self-assigned this Nov 6, 2017
@saki7 saki7 added this to the Sidebar v2 完全移行 milestone Nov 6, 2017
@saki7
Copy link
Contributor Author

saki7 commented Nov 7, 2017

見た目けっこうひどかったので cpprefjp/kunai@df0caf9 で軽く直しました、ついでにスマホも直したのでPCとスマホどちらでも綺麗に見れると思います

screenshot_20171107-091859

@faithandbrave
Copy link
Member

バグレポではない報告です。
iPhone 7のiOS 10のSafariブラウザ環境で、サイドバーのみが表示されメインコンテンツが表示されない問題がありました。iOS 11に上げたら直りました。

それと、バグ報告先はcpprefjp/kunai、と書かれていますが、プロジェクトはなぜかcpprefjp/site側にあるようです。報告先がちょっと曖昧になっている気がします。 @usagi さんがこちらのリポジトリに報告したのもそのせいかな、と。

@saki7
Copy link
Contributor Author

saki7 commented Nov 9, 2017

@faithandbrave iOS 10以前のWebkitの問題だと思います。flex周りでwebkitのベンダープレフィックスをつける変更で包括的に直ると思うので、対応します。

GitHubのProjectは、リポジトリ単位と組織単位の2つあって、Sidebar v2は組織単位で作っています。このProjectを作る前に実験してみたのですが、リポジトリ単位で作ってしまうと別のリポジトリのIssueを全く参照できないようなので、こうなっています。

@faithandbrave
Copy link
Member

あ、Organization GroupごとにProjectsを設定できましたね。忘却の彼方でした。失礼しました。

@saki7
Copy link
Contributor Author

saki7 commented Nov 9, 2017

めちゃくちゃ重かった問題を cpprefjp/kunai#35 で完全解決したのでこれで致命的な問題は(IE11の表示崩れ)以外なくなったと思います

IE11の件はちょっと僕の手に負えないので誰か助けてください。僕の家にIE11を常時起動できるWindowsマシンが無いので、検証自体ができません。今まではブラウザスクリーンショットを取ってくれるサービスを使ってたんですが流石にこれだと限界があります。

@saki7
Copy link
Contributor Author

saki7 commented Nov 9, 2017

あと、Twitterなどで「cpprefjpの新しいデザインが見づらい」という意見が上がっていることは把握しているのですが、具体的にどこがどういう風に見づらいのか僕は分かっていません(設計したのが僕なので)。

見づらいという意見を言っている方々にFF外から失礼するわけにはいかないので、僕はFF外から失礼はしませんし、ここのMemberの方々もそういうことはやらないようにお願いしたいのですが、その一方でやはり具体的に見づらい所がどこかという点は知りたいです。

cpprefp/kunai#35 の遅延評価のついでに実装したツリー展開機能でだいたいの見づらさは解消したと思うのですが、これでもまだ致命的に見づらいと感じる方がもしここを見ていましたら、なるべく詳しく詳細を教えてください。対応します。

@faithandbrave
Copy link
Member

直接報告しないぼやきは意見として拾わなくていいと思いますが、サイドバーについて個人的に思うのは以下です:

  • 展開したところのスクロールで誤操作が起こりやすい
    • 内部スクロールのしすぎでページ全体がスクロールしてしまうのは、操作としてイライラの感情が起こりやすい。ここは、元々のツリーUIに対するディスアドバンテージかな、と思います。
  • リファレンス以外も全て、デフォルトで畳んでほしい
    • とくに編集者向けドキュメントは、HTMLページで読むことを想定しておらず、Markdownの書き方を調べるためのものです。展開してあるのは意図ととしてちょっと困ります。
  • Wandboxでコード実行する部分は、コード例の上部にあるバーが大きくて主張しすぎかな、という気はしています。

ほか、アイコン類も含めて、とくに不満はないです。

@faithandbrave
Copy link
Member

IE11の件は、ぼくもWindows環境が職場にも家にもないので、いまのところはお手伝いできません。報告者の @usagi さんに検証に付き合っていただくか、もしくはほかの協力者が現れない限りは、Issueとして残しておいて対応は保留にするしかないのではないかと思います。

@faithandbrave
Copy link
Member

UIについては、慣れたものから慣れないものへの移行、ずっと使っていたものから新しいものへの移行で、ある程度、思い出補正によって前のほうがよかった、という意見はどのサイトでも起こることです。
「使いにくい」という直接の意見があれば具体的に聞いて解決してよいと思いますが、見た目が気に食わない程度であれば思い出補正である場合があるので、慣れれば不満がでなくなる部分は多くあるでしょう。

@saki7
Copy link
Contributor Author

saki7 commented Nov 9, 2017

  • 展開したところのスクロールで誤操作が起こりやすい
    内部スクロールのしすぎでページ全体がスクロールしてしまうのは、操作としてイライラの感情が起こりやすい。ここは、元々のツリーUIに対するディスアドバンテージかな、と思います。

ああ、これは非常に分かります。ただ、これを防ぐためにはブラウザのウィンドウレベルでグローバルなスクロールキャンセル処理を入れるしかなくて、この方法はWebサイトの実装としてあまりお行儀がよくないので悩んでいます。

これのもう1つの解決法はサイドバー全体を常時左側にposition: fixedで固定することですが、これはサイト自体のコンセプトとしてそういう「左側のサイドバーは常に固定!絶対出ているべき!」という風に主張が激しくなるので、この仕様を採用するとしたらこれは合意形成が必要なレベルな気がしています。

  • リファレンス以外も全て、デフォルトで畳んでほしい

これは実装としては結構手間がかかるのですが、僕もそのうち直そうと思っていたので直したらデプロイします

  • Wandboxでコード実行する部分は、コード例の上部にあるバーが大きくて主張しすぎかな、という気はしています。

分かりました。これは調整します


IE11の件は、ぼくもWindows環境が職場にも家にもないので、いまのところはお手伝いできません。報告者の @usagi さんに検証に付き合っていただくか、もしくはほかの協力者が現れない限りは、Issueとして残しておいて対応は保留にするしかないのではないかと思います。

分かりました。

UIについては、慣れたものから慣れないものへの移行、ずっと使っていたものから新しいものへの移行で、ある程度、思い出補正によって前のほうがよかった、という意見はどのサイトでも起こることです。
「使いにくい」という直接の意見があれば具体的に聞いて解決してよいと思いますが、見た目が気に食わない程度であれば思い出補正である場合があるので、慣れれば不満がでなくなる部分は多くあるでしょう。

そういうことであれば良いんですが、そうではなくデザインとして悪いせいで見づらいという結論が出てしまうと僕としては悲しいですね……

@saki7
Copy link
Contributor Author

saki7 commented Nov 9, 2017

ところで、ツリービューが見づらい件は色々試して遊んでいたらそこそこ良さそうな物が出来たので試しに本番に投入しました。これはこれで人によっては嫌いな人もいる類のUIだとは思うんですが、前と比べるとこっちの方がいい気がしてます

cpprefjp/kunai#36

peek 2017-11-10 01-47

saki7 added a commit to cpprefjp/kunai that referenced this issue Nov 9, 2017
saki7 added a commit to cpprefjp/kunai_config that referenced this issue Nov 9, 2017
saki7 added a commit to cpprefjp/kunai that referenced this issue Nov 9, 2017
@faithandbrave
Copy link
Member

いろいろ対応ありがとうございます。
一点気になったのは、スクロールバーが出ているときに、折りたたみ・展開機能が使えないことですね。いまのところ対応策はノーアイデアで申し訳ないですが・・・。

@yumetodo
Copy link
Member

ツリー展開機能に一つ不満があるとすれば、ツリー展開してスクロール中に項目名が表示し続けてくれるのはいいんですが、スクロールして閉じると、ツリー展開せずにスクロールしたのと同じところまでスクロールされてしまうことでしょうか・・・

閉じる前 閉じた後
image image

@saki7
Copy link
Contributor Author

saki7 commented Nov 10, 2017

@faithandbrave:

一点気になったのは、スクロールバーが出ているときに、折りたたみ・展開機能が使えないことですね。いまのところ対応策はノーアイデアで申し訳ないですが・・・。

これを解決するためにはやはりこれをやるしかないですね

これのもう1つの解決法はサイドバー全体を常時左側にposition: fixedで固定することですが、これはサイト自体のコンセプトとしてそういう「左側のサイドバーは常に固定!絶対出ているべき!」という風に主張が激しくなるので、この仕様を採用するとしたらこれは合意形成が必要なレベルな気がしています。 (@saki7)


@yumetodo:

ツリー展開機能に一つ不満があるとすれば、ツリー展開してスクロール中に項目名が表示し続けてくれるのはいいんですが、スクロールして閉じると、ツリー展開せずにスクロールしたのと同じところまでスクロールされてしまうことでしょうか・・・

直します。ただ、expected behaviorを定義するのが難しいです。気持ち良いスクロールを実現できるためのコーナーケースの条件分岐を定義するのが結構難しい

@faithandbrave
Copy link
Member

サイドバーを左固定にしていないのは、以下のIssueによるものなので、 @melpon と相談して決めていただければ。
#242

@saki7
Copy link
Contributor Author

saki7 commented Nov 10, 2017

@faithandbrave なるほど。こういう理由でしたか。元々左固定だったとのことで、その時点で不満が出ていなくて、幅が可変になった時にレイアウトの問題があったということですね。

それなら、これはCSSのメディアクエリとFlexboxのflex-directionの切り替え1発でしっかり調整できるので、いっそのことサイトのコンセプトレベルでサイドバーを左固定にしようと思います。

@yumetodo
Copy link
Member

yumetodo commented Nov 10, 2017

IEじゃなくてEdgeですね。Edgeで追うのはだるいのでとりあえずhttp://127.0.0.1:8080/js/browser.js?3b68057c3554b8ae107eとやらを手元のテキストエディタにコピペして現在追っています。

Unhandled promise rejection TypeError: Unable to get property 'index' of undefined or null reference


https://github.com/cpprefjp/crsearch/blob/cbd0eb1d43901518e9322649cc7c7c988e42c230/js/crsearch/database.js#L244
から飛んでますね(http://127.0.0.1:8080/js/browser.js?3b68057c3554b8ae107e:14383:9に該当)
こっちのほうが致命的な方のエラー。

@yumetodo
Copy link
Member

yumetodo commented Nov 10, 2017

Unhandled promise rejection TypeError: Unable to get property 'set' of undefined or null reference


https://github.com/cpprefjp/crsearch/blob/fec29874588110d9ad6cf87374c2b4f8ef452698/js/crsearch/crsearch.js#L277
から飛んでます(http://127.0.0.1:8080/js/browser.js?3b68057c3554b8ae107e:39868:7に該当)


http://127.0.0.1:8080/js/browser.js?3b68057c3554b8ae107eを貼っておきます、念のため.

https://gist.githubusercontent.com/yumetodo/b297c08c26871bfbd3fb513758941da3/raw/8d63efbd2db1a334269de44f5a725fd01fb3730c/browser.js

@saki7
Copy link
Contributor Author

saki7 commented Nov 10, 2017

ああこの2個目はURLオブジェクトをnewできないといかいうアレじゃないでしょうか

1個目はL244の前に

console.log(a); console.log(b);

入れてください

@yumetodo
Copy link
Member

yumetodo commented Nov 10, 2017

console.logを差し込むとどういうわけか再現率が1/10程度に下がる・・・

image

(今更だけど別Issueで話すべきだったかもしれないという思い)

@saki7
Copy link
Contributor Author

saki7 commented Nov 10, 2017

L234の前に

console.log(ns)
console.log(ns.namespace)
console.log(kc)
console.log(kc.categories().has(ns.namespace[0]))

@yumetodo
Copy link
Member

ああこの2個目はURLオブジェクトをnewできないといかいうアレじゃないでしょうか

#477 (comment)
これ?
Edgeだとnewはできるんだけど(build 10240以降)searchParamsがないんですね
https://developer.microsoft.com/en-us/microsoft-edge/platform/catalog/?q=specName%3Awhatwg-url

@yumetodo
Copy link
Member

image
image

@saki7
Copy link
Contributor Author

saki7 commented Nov 10, 2017

僕がここで何をやっているのか説明するので分かる範囲で色々試してみてください

crsearchはまずgetTree() の前に cpprefjp/kunai_config のjson/md からcpprefjpのグローバル設定を取ります それを格納してるクラスのオブジェクトが kc

ここでの実際の処理は「crsearch.json内の全てのトップレベル記事カテゴリをcpprefjp/kunai_configの定義通りにソートする」処理です。で、
kc.categories() っていうのは article.md の中で定義してある「cpprefjpのサイト全体のカテゴリ一覧」の名前をキーとして値にソート順序の数値が入った連想配列です。キーは 「リファレンス」だったら 'reference' 「言語機能」だったら 'lang' ですがこれは ns.namespace[0] と必ず一致するはずです。なので kc.categories().get(ns.namespace[0]) していますが、これが null になっていると いわゆる致命的なエラーが出ます

@yumetodo
Copy link
Member

yumetodo commented Nov 10, 2017

もう夜が明けそうなので明日追ってみます

kc.categories().has(ns.namespace[0])が全部falseなのはなんかおかしいように思っていますが・・・というかkc.categories().sizeが全部0...

@saki7
Copy link
Contributor Author

saki7 commented Nov 10, 2017

@yumetodo 色々考えてたら原因浮かびました

現行のJavaScriptではネイティブなクラスをMapのキーに出来ないのでところどころでHarmonyのWeakMapを使っているのですが、WeakMapは弱参照なので全ての参照が途切れたタイミングでGCされる可能性があります

Chromeだと今まで一度もエラーが起きていないので、Edgeが多分GCのタイミングがシビアなんでしょう。たぶん合ってると思います。解決策としては連想配列を全て諦めて文字列をキーにしてデータ構造を作り直すくらいですかね。

@yumetodo
Copy link
Member

https://github.com/cpprefjp/kunai/blob/f11786ad8c9a434272a907efaf032d774326d784/js/kunai/ui/sidebar.js#L10-L13
の時点でconsole.log("sidebar.js:13==>", this.kc);した結果
image
こうなるのは説明が付きますかね・・・?

@saki7
Copy link
Contributor Author

saki7 commented Nov 11, 2017

@yumetodo ん、合ってますね。数も正しいと思います

@saki7
Copy link
Contributor Author

saki7 commented Nov 11, 2017

おはようございます、1つ新機能を実装して3つバグを生成しました

とりあえずありがたいことに @yumetodo さんがEdge周りをデバッグしてくれているので便宜上担当者として設定しておきます

@saki7
Copy link
Contributor Author

saki7 commented Nov 11, 2017

便宜上という表現は失礼でした、僕の意図としては担当者に設定しますが基本ボランティアなので義務感を感じたら気軽にアサインは外していいということなので……

@yumetodo
Copy link
Member

https://github.com/cpprefjp/crsearch/blob/fec29874588110d9ad6cf87374c2b4f8ef452698/js/crsearch/kunai-config.js#L138

console.log("kunai-config.js:138==>", e)

を追記してlogを取った結果

表示されない時 表示される時
image image

これは

const e = Config.parseMD(data['article.md'], new ArticleProcessor)

が死んでる・・・?

@yumetodo
Copy link
Member

yumetodo commented Nov 11, 2017

@saki7
https://github.com/cpprefjp/crsearch/blob/fec29874588110d9ad6cf87374c2b4f8ef452698/js/crsearch/kunai-config.js#L129-L131

lexer.lex(md_raw).map(e => new Map(Object.entries(e)))はなんかどっさり色々返してきているので問題はproc.processの中だと思うんですが、何をどうしたらいいやら・・・

@saki7
Copy link
Contributor Author

saki7 commented Nov 11, 2017

@yumetodo new ArticleProcessorを渡してる部分でこれを変数に保存してなくてそのまま一時オブジェクトとして渡してるのが怪しいので、変数として取っておいて中身をconsole.logするとか(JSにはprivateとかいう概念は無いのでメンバ変数は外からアクセスできる)

@yumetodo
Copy link
Member

@saki7

yumetodo/kunai@1ddd372
の環境で検証したところ

image

unhandled heading 'TOPLEVEL_CATEGORY'
unhandled heading 'GLOBAL_QUALIFY_LIST'

が飛んでます

yumetodo referenced this issue in yumetodo/crsearch Nov 11, 2017
yumetodo referenced this issue in yumetodo/kunai Nov 11, 2017
yumetodo referenced this issue in cpprefjp/crsearch Nov 11, 2017
@yumetodo
Copy link
Member

#476 (comment)
Unhandled promise rejection TypeError: Unable to get property 'set' of undefined or null reference

の件は cpprefjp/crsearch@933a306 で解決しました

@saki7
Copy link
Contributor Author

saki7 commented Nov 11, 2017

当初のマイルストーンの設定通り、修正項目は今日の時点でリリースに向けてクローズします。

@saki7
Copy link
Contributor Author

saki7 commented Nov 12, 2017

対応は終了

@saki7 saki7 closed this as completed Nov 12, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants