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

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

Closed
thiagowfx opened this issue Jul 5, 2023 · 15 comments · Fixed by #522
Closed
Assignees
Labels
bug Something isn't working module-browsingContext Browsing Context module

Comments

@thiagowfx
Copy link
Member

thiagowfx commented Jul 5, 2023

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:

  • width is zero
  • height is zero
  • left margin plus right margin is greater than or equal to width (=effectively zero width)
  • top margin plus bottom margin is greater than or equal to height (=effectively zero height)

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 thiagowfx added module-browsingContext Browsing Context module needs-discussion Issues to be discussed by the working group labels Jul 5, 2023
@thiagowfx thiagowfx self-assigned this Jul 5, 2023
@thiagowfx
Copy link
Member Author

thiagowfx commented Jul 5, 2023

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

@whimboo
Copy link
Contributor

whimboo commented Jul 5, 2023

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

@OrKoN
Copy link
Contributor

OrKoN commented Jul 5, 2023

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
Copy link
Member

jgraham commented Jul 5, 2023

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
Copy link
Member Author

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
Copy link
Contributor

whimboo commented Jul 5, 2023

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:

w3c/csswg-drafts#8335

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

@thiagowfx
Copy link
Member Author

thiagowfx commented Jul 6, 2023

@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
Copy link
Member Author

thiagowfx commented Jul 10, 2023

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
Copy link
Member Author

Updated the WPT tests in the direction this thread is going to: web-platform-tests/wpt#40872

@whimboo
Copy link
Contributor

whimboo commented Aug 2, 2023

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

@thiagowfx
Copy link
Member Author

@whimboo We have already decided the direction of this topic. Unfortunately css-meeting-bot posted the minutes to the wrong issue, see them here: #481 (comment)

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

@whimboo
Copy link
Contributor

whimboo commented Aug 2, 2023

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 thiagowfx added bug Something isn't working and removed needs-discussion Issues to be discussed by the working group labels Aug 23, 2023
@thiagowfx
Copy link
Member Author

Recap of #481 (comment):

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
Copy link
Member Author

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
Copy link
Member Author

WPT: web-platform-tests/wpt#40872

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working module-browsingContext Browsing Context module
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants