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

Update browsingContext.print command with semantic types #481

Closed
wants to merge 1 commit into from

Conversation

jrandolf-2
Copy link
Contributor

@jrandolf-2 jrandolf-2 commented Jul 7, 2023

This PR does a few things:

  • Renames page, pageRanges, browsingContext.PrintMarginParameters, and browsingContext.PrintPageParameters to dimensions, range, browsingContext.PageMargin, and browsingContext.PageDimensions.
  • Removes the text production for range in favor of the less complicated browsingContext.PageRange.
  • Adds an algorithm for getting the set of pages from the range

The motivation for this PR is twofold:

  1. The names of some of the parameters are not clear on inspection.
  2. The algorithm for pageRanges is overly complicated for something that results in the production described in this PR.

Preview | Diff

@jgraham jgraham added the needs-discussion Issues to be discussed by the working group label Jul 7, 2023
@jgraham
Copy link
Member

jgraham commented Jul 7, 2023

I think changing the command syntax needs broader input from the WG.

This PR does a few things:

 - Renames `page`, `pageRanges`, `browsingContext.PrintMarginParameters`, and ` browsingContext.PrintPageParameters` to `dimensions`, `range`, `browsingContext.PageMargin`, and `browsingContext.PageDimension`.
  - Removes the `text` production for `range` in favor of the less complicated `browsingContext.PageRange`.
  - Adds an algorithm for getting the set of pages from the `range`

The motivation for this PR is twofold:

 1. The names of some of the parameters are not clear on inspection.
 2. The algorithm for `pageRanges` is overly complicated for something that results in the production described in this PR.
Copy link
Contributor

@OrKoN OrKoN left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM as it makes the renaming makes the parameters somewhat more readable, in my opinion. Deferring to @jgraham for the final review as he has more context about the current naming.

Copy link
Member

@thiagowfx thiagowfx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please wait for more reviewers.

@jrandolf-2
Copy link
Contributor Author

As discussed offline, we won't move forward with this.

@jrandolf-2 jrandolf-2 closed this Jul 12, 2023
@css-meeting-bot
Copy link
Member

The Browser Testing and Tools Working Group just discussed Printing.

The full IRC log of that discussion <jgraham_> Topic: Printing
<jgraham_> GitHub: https://github.com//issues/473
<orkon> thiagowfx: I opened an issue that when the page dimensions are effectively zero (explicit setting or via margins). Currently the spec does not rule this out and there is a WPT test for that.
<jgraham_> q+
<orkon> thiagowfx: the expected result is to generate blank PDF with no content. This happens to work in Firefox but Chromium is not capable of producing empty content PDFs. Given that it is not a very useful scenario my proposal is to change the spec and WPT in some way: 1) rule out the case with empty dimensions with an error (InvalidArgument or smth) 2) let it work but allow implementations to fail on this case (UnsupportedOperation). what is the
<orkon> preference?
<jgraham_> ack jgraham_
<mathiasbynens> present-
<orkon> jgraham_ (IRC): in the spec at the moment you already allowed to send unsupported operation. But of course tests do not always assume. It is a bit tricky: we want the spec to have this clause and dont want to define the very specific implementation. Is it really only zero or some minimal size?
<orkon> jgraham_ (IRC): Henrik mentioned that there is some minimal size like 0.1 inch
<orkon> jgraham_ (IRC): if we define that, we can throw an invalid arg
<orkon> thiagowfx: are you advocating for not changing the spec?
<orkon> jgraham_ (IRC): we should change the spec for literal zero? the only question if the limit should be literal zero
<orkon> thiagowfx: the threshold will probably be 1 x 1 point
<orkon> jgraham_ (IRC): we should get that to the spec then
<jrandolf> q+
<thiagowfx> present-
<jgraham_> github: https://github.com//pull/481
<jgraham_> ack jrandolf
<orkon> jrandolf: how do we feel about changing the names of properties to be more semantic? we took from WebDriver and it was built over time so it perhaps makes sense to rename some things, for certain params, like what does page mean? it stands for page dimensions
<orkon> jgraham_ (IRC): dimensions probably is a slight improvement, pages vs pageRanges perhaps not so much. given that the clients can expose it in whatever way, I am not sure it makes sense to add breaking changes
<jrandolf> Yes
<orkon> jgraham_ (IRC): another change was to require ranges to not require string parsing
<orkon> jgraham_ (IRC): I think it is a more interesting proposal as it removes some string parsing from strings
<orkon> jgraham_ (IRC): we didn't do it differently because it works like in the print dialog
<orkon> jgraham_ (IRC): I would be more inclined to change that
<orkon> jgraham_ (IRC): but I am slightly nervous about slight changes that do not add too much value
<orkon> simonstewart (IRC): I think we payloads will be compatible with classic we should keep the names. If BiDi part is unique, we should do whatever we want
<orkon> jrandolf: I guess then we dont do it
<orkon> present-
<jgraham_> RRSAgent: make minutes
<RRSAgent> I have made the request to generate https://www.w3.org/2023/07/12-webdriver-minutes.html jgraham_
<jgraham_> Zakim, bye
<Zakim> leaving. As of this point the attendees have been mathiasbynens, JimEvans, orkon, jrandolf, brwalder, jgraham, sasha, ChristianBromann, thiagowfx, lolaodelola, simonstewart
<jgraham_> RRSAgent: bye
<RRSAgent> I see no action items
<orkon> unfortunately we didn't reach the cookies agenda item. Should we schedule some other opportunity to discuss it?
<orkon> cc mathiasbynens (IRC)
<jgraham> So I think the key thing for cookies would be a proposal for how to handle storage partitioning / multi-keyed cookies.... (full message at <https://matrix.org/_matrix/media/v3/download/matrix.org/HxnFseNNTrFPcbnsmdmHgUGH>)
<jgraham> And of course different browsers have different partitioning, so any logic is going to be browser (and probably browser-version) specific.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-discussion Issues to be discussed by the working group
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants