w3c / webdriver-bidi

Bidirectional WebDriver protocol for browser automation
https://w3c.github.io/webdriver-bidi/
353 stars 39 forks source link

`browsingContext.print`: should emit `InvalidArgument` when printing an empty content area #473

Closed thiagowfx closed 1 year ago

thiagowfx commented 1 year ago

The test_margin_same_as_page_dimension test in webdriver/tests/bidi/browsing_context/print /margin.py in WPT currently emits a PDF and checks that it has empty content:

@pytest.mark.parametrize(
    "margin",
    [
        {"top": 27.94},
        {"left": 21.59},
        {"right": 21.59},
        {"bottom": 27.94},
        {"top": 27.94, "left": 21.59, "right": 21.59, "bottom": 27.94},
    ],
    ids=[
        "top",
        "left",
        "right",
        "bottom",
        "all",
    ],
)
async def test_margin_same_as_page_dimension(
    bidi_session,
    top_context,
    inline,
    assert_pdf_content,
    margin,
):
    page = inline("Text")
    await bidi_session.browsing_context.navigate(
        context=top_context["context"], url=page, wait="complete"
    )
    print_value = await bidi_session.browsing_context.print(
        context=top_context["context"],
        shrink_to_fit=False,
        margin=margin,
    )

    # Check that content is out of page dimensions.
    await assert_pdf_content(print_value, [{"type": "string", "value": ""}])

For context, this happens to works on Firefox, but it does not work on Chromium, but this is not the point (the spec should be browser agnostic). On Chromium, it throws webdriver.bidi.error.UnknownErrorException: unknown error (invalid print parameters: content area is empty), which is technically correct. Chromium cannot handle printing an empty content area.

Putting vendors aside, the purpose of this issue is to question the validity of printing a page in either of these scenarios, which all produce an empty content area:

My understanding is that currently the BiDi spec implicitly allows this condition (@whimboo seems to agree).

That said, with further investigation...

@whimboo (source)

Hm, looks like that I may be wrong here. Searching for a 0-sized PDF it looks like that the minimum valid size for a PDF page is 1 point, which is equivalent to approximately 0.353 mm or 0.0139 inches. Sadly I cannot find any download which is free of charge. As such I would propose to file a BiDi spec issue first for further discussion.

A comment that I cannot verify:

In Section 14.11.2 of the PDF specification, it states that the width and height of a page in a PDF document shall be specified as positive numbers, and values of zero or less are considered invalid. This implies that a PDF page must have dimensions greater than zero.

Hereby my proposal is to amend https://w3c.github.io/webdriver-bidi/#command-browsingContext-print with the following change (exact wording TBD, not sure how to express this in the precise way the spec requires):

If the content area is effectively empty (by the means of an effective width or height equal to zero), send an error response with code invalid argument.

Then we could change the WPT test accordingly.

Thoughts?

thiagowfx commented 1 year ago

cc (at least) @whimboo @OrKoN to have review from two distinct vendors.

whimboo commented 1 year ago

Given that I'm out soon I would forward this to @jgraham and @lutien.

OrKoN commented 1 year ago

If the implementation is unable to provide a paginated representation of context for any reason then return error with error code unsupported operation.

would this clause apply for this case? If I understood the issue right, I would be in favour of changing the spec to indicate that printing empty content might not be supported and change the test. Unless there is a use case for doing those print operations?

jgraham commented 1 year ago

These "for any reason" clauses mean that strictly any test for the feature could be allowed to throw "unsupported operation"

However practically we don't do that, and it probably wouldn't be helpful in general (e.g. it would make it look like an implementaton that always threw unsupported operation was conforming, which isn't the intent). But I think "because the page is zero size" is a totally reasonable reason to not generate a PDF and we could test for in the spec directly. Is it literally zero size that's the problem, or any size smaller than some value?

Alternatively, if we don't want a spec change, we could adjust the specific tests to also pass if there's an "Unsupported Operation" exception.

thiagowfx commented 1 year ago

Is it literally zero size that's the problem, or any size smaller than some value?

Let me make a few experiments and get back to this thread.

Alternatively, if we don't want a spec change, we could adjust the specific tests to also pass if there's an "Unsupported Operation" exception.

Shouldn't this be "Invalid Argument"?

whimboo commented 1 year ago

Please note that there is also some unclarity how a page size of 0 should be handled in general. There is the following csswg-drafts issue:

https://github.com/w3c/csswg-drafts/issues/8335

There is also a bug in Firefox which covers this current issue that we allow printing of a 0x0 sized content area.

thiagowfx commented 1 year ago

@jgraham:

Is it literally zero size that's the problem, or any size smaller than some value?

@thiagowfx:

Let me make a few experiments and get back to this thread.

https://chromium-review.googlesource.com/c/chromium/src/+/4611587 (and underlying crbug):

For tiny pages this results in at least 1x1 pt page dimensions.

From the PR comment above, it seems that only literally zero size is the problem.

thiagowfx commented 1 year ago

Do you think it's worth to bring this topic to the monthly WG meeting?

I'd just like to know what the next steps are, and it seems we don't yet have a consensus.

I believe the AIs are:

1) Decide whether or not to explicitly mention in the BiDi spec that zero dimensions are unsupported. (Yes/No) 2) Update WPT tests to properly handle errors for this scenario. Decision: (InvalidArgument/UnsupportedOperation)?

  1. Should the WPT tests assume the implementation could work, like Firefox's current one? (Yes/No)
thiagowfx commented 1 year ago

Updated the WPT tests in the direction this thread is going to: https://github.com/web-platform-tests/wpt/pull/40872

whimboo commented 1 year ago

@thiagowfx would you mind bringing up this topic in the next monthly meeting?

thiagowfx commented 1 year ago

@whimboo We have already decided the direction of this topic. Unfortunately css-meeting-bot posted the minutes to the wrong issue, see them here: https://github.com/w3c/webdriver-bidi/pull/481#issuecomment-1633030784

There are currently no blockers for it, I just didn't have time to start it yet.

whimboo commented 1 year ago

I see. Thanks for pointing that out. Given that I wasn't around I missed that it has been discussed. As it looks like we are going to not allow sizes smaller than 1x1 pixels.

thiagowfx commented 1 year ago

Recap of https://github.com/w3c/webdriver-bidi/pull/481#issuecomment-1633030784:

The threshold will [...] be 1 x 1 point

If less than that, throw either InvalidArgument or UnsupportedOperation. The former seems to make more sense.

In order to yield 1 point, we use this formula:

px = cm * 96 / 2.54

...wherein 96 is the PPI.

For 1px (point), the cm equivalent is 2.54 / 96 = 0.02645833333, that's the value we have to use in the spec.

Is it appropriate to assume PPI=96 in the spec?

thiagowfx commented 1 year ago

Answering my own question: yes, it's appropriate to assume so, as per the W3C definition: https://www.w3.org/TR/css3-values/#absolute-lengths

The question is now whether the minimum should be 1x1 point or 1x1 px. I hadn't realized there were two distinct units and were treating them as synonyms.

If we do pixels, then we want the minimum possible cm to be 2.54 / 96 = 0.02645833333

If we do points, then it is: 2.54 / 72 = 0.03527777777.

thiagowfx commented 1 year ago

WPT: https://github.com/web-platform-tests/wpt/pull/40872