Open Bug 1561401 Opened 5 years ago Updated 2 years ago

Add tests to cover DOM.getBoxModel and getContentQuads

Categories

(Remote Protocol :: CDP, task, P3)

task

Tracking

(Not tracked)

People

(Reporter: ochameau, Unassigned)

References

(Blocks 2 open bugs)

Details

Attachments

(2 obsolete files)

In bug 1545726 and 1543151 I introduced the implementation of DOM.getBoxModel and DOM.getContentQuads.
But I've not been able to have a valid test passing on all platforms.

The main issue is around high-dpi.
The numbers returned by platfom API used by the implementation (getBoundingClientRect and getBoxQuads) starts returning decimals instead of numbers.
This is a first issue. I don't know if chromium returns decimals numbers. In the landed implementation I rounded all the returned numbers.
The second issue is about being able to assert the returned values of these CDP methods.
As the result is unique per platform it doesn't sounds right to assert hardcoded values.
And I haven't found anything that would compute the expected value without reusing the exact same API in the test...

Attached file Bug 1561401 - Add tests for DOM.getContentQuads. (obsolete) (deleted) —
Attached file Bug 1561401 - Add tests for DOM.getBoxModel. (obsolete) (deleted) —

Brad, I'm having issues testing this code which relies on getBoxQuads.
The final values are different between linux, osx and windows.
I assumed it was because of DPI being different between each environment on try.
I saw that you were involved into getBoxQuads, and, I was wondering if you know
if I could better control the environment in order to have the same values on all the platforms?
Like changing the dpi, tweaking a pref, a windowUtils flag or something.

Otherwise, the only way to assert the final value would be to use the exact same implementation in the test,
or have loose values to assert.

Flags: needinfo?(bwerth)

I'm applying your patches and will step through to see if anything seems touchy about how getBoxQuads evaluates the frames. If I can't find anything, I'll pass this on to someone on the layout team.

(In reply to Alexandre Poirot [:ochameau] from comment #3)

Brad, I'm having issues testing this code which relies on getBoxQuads.
The final values are different between linux, osx and windows.
I assumed it was because of DPI being different between each environment on try.
I saw that you were involved into getBoxQuads, and, I was wondering if you know
if I could better control the environment in order to have the same values on all the platforms?
Like changing the dpi, tweaking a pref, a windowUtils flag or something.

I'm sorry, but I'm not able to find a good reason why getBoxQuads would return varying values on different platforms. AccumulateQuadCallback::AddBox [1] is just getting the frames in nsPoint units (app units) and converting them to CSS pixels, which is done at a fixed rate of 60 units per pixel. DPI never enters into it. I'm sending the needinfo on to Emilio who will likely have more insight.

I'm sure it would be helpful to post the try server push so we can see what the variance is on the other platforms. My local tests on macOS passed without trouble. Are the failing values off by a constant (indicating a translation error) or off by a linear factor (indicating a scaling error)?

[1] [https://searchfox.org/mozilla-central/source/layout/base/GeometryUtils.cpp#178]

Flags: needinfo?(bwerth) → needinfo?(emilio)

Looking at that test it seems to be trying to guess the line box height which depends on the font metrics, which do vary per platform. Does using something like font: 16px/1 monospace or such help?

Flags: needinfo?(emilio)

(In reply to Emilio Cobos Álvarez (:emilio) from comment #6)

Looking at that test it seems to be trying to guess the line box height which depends on the font metrics, which do vary per platform. Does using something like font: 16px/1 monospace or such help?

Yes, I'm trying to guess the box height of individual lines. But also the coordinates of each, which may be the main issue.

With the following html:

<div style="position: absolute; top: 5px; left: 10px; width: 50px; height: 100px; background: silver; padding: 0px;">
  <span style="font: 24px/1 monospace;">foo foo foo</span>
</div>

I would expect <span> to be at (10, 5). But instead, getBoxQuads will report a lower Y.
Here on Windows with a "zoomed (250%) high resolution screen", I get (10, 3.4000000953674316).
Note that I lack vocabulary here. I don't know really what DPI means in the context of high resolution screen where you zoom it.
Also this zoom you set in your OS in order to increase the UI/font sizes must have a precise name, which I don't know.

Using font: 24px/1 monospace; doesn't help getting better coordinates.

Here is a test case to verify the values on your environment:
data:text/html;charset=utf-8,<div style="position: absolute; top: 5px; left: 10px; width: 50px; height: 100px; background: silver; padding: 0px;"><span style="font: 24px/1 monospace;">foo foo foo</span></div><script>console.log(document.querySelector("span").getBoxQuads()[0].p1)</script>

Hmm, so if you give the <span> a display value of inline-block, then the quad positions are what you expect... I'm not totally familiar with how vertical positioning of inlines vs. inline-blocks works by default, but Chrome seems to do the same actually... That's due to the baseline vertical-alignment of the span (so it actually depends on the baseline of the font). Using inline-block you should be able to use vertical-align to override the alignment I think.

Whiteboard: [puppeteer-alp
Whiteboard: [puppeteer-alp → [puppeteer-alpha]
Whiteboard: [puppeteer-alpha] → [puppeteer-alpha-reserve]
Priority: P2 → P3
Priority: P3 → P2
Whiteboard: [puppeteer-alpha-reserve] → [puppeteer-beta-mvp]
Priority: P2 → P3
Whiteboard: [puppeteer-beta-mvp] → [puppeteer-beta-reserve]
Assignee: nobody → poirot.alex
Status: NEW → ASSIGNED

That wasn't me but Phabricator. Resetting the assignee, which will hopefully stick.

Assignee: poirot.alex → nobody
Status: ASSIGNED → NEW
Assignee: nobody → poirot.alex
Attachment #9073921 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #9073920 - Attachment is obsolete: true
Priority: P3 → P1
Whiteboard: [puppeteer-beta-reserve] → [puppeteer-beta-reserve]

No this bug isn't assigned. Alexandre only abandoned patches he was working on months ago. So reverting to the former state.

Assignee: poirot.alex → nobody
Status: ASSIGNED → NEW
Priority: P1 → P3
Whiteboard: [puppeteer-beta-reserve]
Component: CDP: DOM → CDP
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: