Skip to content

/ servo

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

Jump to bottom

Canvases elements have extraneous padding #10726

Open
jdm opened this issue Apr 19, 2016 · 7 comments
Open

Canvases elements have extraneous padding #10726

jdm opened this issue Apr 19, 2016 · 7 comments
Labels
A-layout/inline C-has-manual-testcase

Comments

@jdm
Copy link
Member

@jdm jdm commented Apr 19, 2016 •
edited

Firefox:
screen shot 2016-04-19 at 6 49 59 pm
Servo:
screen shot 2016-04-19 at 6 50 16 pm

<style>
canvas {
  border: 1px solid black;
  margin: 0;
  padding: 0;
}
span {
  width: 32px;
  height: 32px;
  display: inline-block;
  border: 1px solid black;
}
</style>
<canvas width="32" height="32"></canvas><br><canvas width="32" height="32"></canvas>
<div></div>
<span></span><br><span></span>
@jdm
Copy link
Member Author

@jdm jdm commented Apr 19, 2016

With an encapsulating div that forces a particular height it looks slightly better:
Firefox:
screen shot 2016-04-19 at 6 51 31 pm
Servo:
screen shot 2016-04-19 at 6 51 26 pm

<style>
canvas {
  border: 1px solid black;
  margin: 0;
  padding: 0;
}
span {
  width: 32px;
  height: 32px;
  display: inline-block;
  border: 1px solid black;
}
</style>
<div style="height: 32px"><canvas width="32" height="32"></canvas></div><canvas width="32" height="32"></canvas>
<div></div>
<span></span><br><span></span>
@jdm jdm mentioned this issue Apr 20, 2016
Merged
@notriddle notriddle self-assigned this Apr 27, 2016
@notriddle
Copy link
Contributor

@notriddle notriddle commented Apr 28, 2016

Those "margins" are actually the space intended for the font's descenders. In both Chrome, these canvases are touching:

<style>
canvas {
  border: 1px solid black;
  margin: 0;
  padding: 0;
}
</style>
<canvas width="32" height="32"></canvas><br><canvas width="32" height="32"></canvas>

image

But these canvases have a "margin" between them:

<style>
canvas {
  border: 1px solid black;
  margin: 0;
  padding: 0;
}
</style>
<canvas width="32" height="32"></canvas>no descenders<br><canvas width="32" height="32"></canvas>

image

In Servo, the space between them is identical.
@notriddle
Copy link
Contributor

@notriddle notriddle commented Apr 28, 2016

The culprit code is InlineFlow::compute_minimum_ascent_and_descent. It seems to mine the font metrics for minimum values even when there is no actual text (also, its comment claims that it works on a line-by-line basis, but it seems to give a result to the entire flow).
@notriddle
Copy link
Contributor

@notriddle notriddle commented May 4, 2016

Since #10691 landed, @jdm's original test case now looks like this:

image
@notriddle notriddle mentioned this issue May 19, 2016
Closed
4 of 4 tasks complete
bors-servo added a commit that referenced this issue May 20, 2016
@bors-servo
Auto merge of #11275 - notriddle:canvas_no_descender, r=mbrubeck
f319391 
Do not put any descenders in unless there's actually text.

- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy --faster` does not report any errors
- [X] These changes fix #10726 (github issue number if applicable).
- [X] There are tests for these changes

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11275)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Jun 2, 2016
@bors-servo
Auto merge of #11275 - notriddle:canvas_no_descender, r=<try>
85232a6 
Do not put any descenders in unless there's actually text.

- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy --faster` does not report any errors
- [X] These changes fix #10726 (github issue number if applicable).
- [X] There are tests for these changes

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11275)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Jun 2, 2016
@bors-servo
Auto merge of #11275 - notriddle:canvas_no_descender, r=<try>
4bfb5be 
Do not put any descenders in unless there's actually text.

- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy --faster` does not report any errors
- [X] These changes fix #10726 (github issue number if applicable).
- [X] There are tests for these changes

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11275)
<!-- Reviewable:end -->
@notriddle notriddle mentioned this issue Jun 18, 2016
Open
@notriddle
Copy link
Contributor

@notriddle notriddle commented Jun 18, 2016 •
edited

Firefox matches Servo's current behavior when a <!DOCTYPE html> declaration is added.

That's why I couldn't find anything in the base CSS spec to justify the behavior; it's in the Quirks Mode spec! That's also why I found tests that seemed to rely on the behavior that this issue reports as a bug; because the tests are running with DOCTYPEs!

:headdesk:
@notriddle
Copy link
Contributor

@notriddle notriddle commented Jun 18, 2016 •
edited 

<!DOCTYPE html>
<style>
canvas {
  border: 1px solid black;
  margin: 0;
  padding: 0;
}
span {
  width: 32px;
  height: 32px;
  display: inline-block;
  border: 1px solid black;
}
</style>
<canvas width="32" height="32"></canvas><br><canvas width="32" height="32"></canvas>
<div></div>
<span></span><br><span></span>

screenshot from 2016-06-17 21-54-15

screenshot from 2016-06-17 21-55-03

screenshot from 2016-06-17 21-58-13
@notriddle notriddle removed their assignment Jun 18, 2016
@jdm jdm removed the C-assigned label Jun 18, 2016
@dralley
Copy link
Contributor

@dralley dralley commented Apr 1, 2020

The original minimal reproducer still works -- didn't try the others.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-layout/inline C-has-manual-testcase
Projects
None yet
Milestone
No milestone
Linked pull requests

Successfully merging a pull request may close this issue.

Do not put any descenders in unless there's actually text.
3 participants
@jdm @notriddle @dralley
You can’t perform that action at this time.