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

Request reviews of baseline tests for `vertical-lr` #21124

Closed
kojiishi opened this issue Jan 10, 2020 · 6 comments
Closed

Request reviews of baseline tests for `vertical-lr` #21124

kojiishi opened this issue Jan 10, 2020 · 6 comments

Comments

@kojiishi
Copy link
Contributor

@kojiishi kojiishi commented Jan 10, 2020

As far as I reviewed, the reference files for these tests need to fix:

external/wpt/css/css-writing-modes/inline-block-alignment-003.xht
external/wpt/css/css-writing-modes/inline-block-alignment-005.xht
external/wpt/css/css-writing-modes/inline-block-alignment-007.xht
external/wpt/css/css-writing-modes/inline-table-alignment-003.xht
external/wpt/css/css-writing-modes/inline-table-alignment-005.xht

A bit of background:

  • When I implemented LayoutNG, these look incorrect and that I deferred fixing them, wanting the second opinions.
  • Reviewed with @fantasai in September, IIRC we agreed that the test needs fix, but don't remember we reviewed them all or only some of them.
  • We're currently trying to change our baseline implementation for some reasons, and are wondering whether to fix tests or to fix our code.

@fantasai Do you remember, or could you review again?

I don't know Hajime and Gerard's github account, I'll send e-mail to them.

cc @bfgeek

@kojiishi
Copy link
Contributor Author

@kojiishi kojiishi commented Jan 10, 2020

Reviewed again but now tests look correct. Sorry if this was a false alarm in advance.

@hshiozawa
Copy link
Contributor

@hshiozawa hshiozawa commented Jan 10, 2020

Hi @kojiishi

Now I have reviewed these tests. But it will take a time to remember my previous work...
Please give me some time.

@hshiozawa
Copy link
Contributor

@hshiozawa hshiozawa commented Jan 11, 2020

Hello @kojiishi

external/wpt/css/css-writing-modes/inline-block-alignment-003.xht
external/wpt/css/css-writing-modes/inline-block-alignment-005.xht
external/wpt/css/css-writing-modes/inline-block-alignment-007.xht
external/wpt/css/css-writing-modes/inline-table-alignment-003.xht
external/wpt/css/css-writing-modes/inline-table-alignment-005.xht

I think that these tests are correct.

These tests are for 'vertical-lr'.
So a rendering of these tests should be equal to flipped of tests for 'vertical-rl' (e.g. external/wpt/css/css-writing-modes/inline-block-alignment-002.xht).

Reviewed again but now tests look correct.

Do you think that these tests are correct now?

@kojiishi
Copy link
Contributor Author

@kojiishi kojiishi commented Jan 14, 2020

Thank you for taking a look, Hajime and Gerard. I think this was a false alarm, sorry for the noise.

I think my memory is coming back -- @fantasai and I reviewed them because there were no impls that pass, and concluded tests are correct. That might be why we didn't take any actions after the review.

@kojiishi kojiishi closed this Jan 14, 2020
@TalbotG
Copy link
Contributor

@TalbotG TalbotG commented Jan 14, 2020

there were no impls that pass

Issue 664386: Baselines of inline-block and inline-table do not respect the last line box or the first row if 'writing-mode' is 'vertical-lr'.
https://bugs.chromium.org/p/chromium/issues/detail?id=664386

Bug 1179952: Baseline alignment of inline-block and inline-table when dominant baseline is central
https://bugzilla.mozilla.org/show_bug.cgi?id=1179952

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.