| 10:52 | <zcorpan> | Noam Rosenthal: why does https://html.spec.whatwg.org/#prepare-an-image-for-presentation return early in several steps, without getting to one of step 14 or 15? Also step 16 is only run if no other step returns. |
| 10:53 | <Noam Rosenthal> | will take a look. Haven't touched that since 2020. |
| 10:57 | <Noam Rosenthal> | Regarding step 16 - it's perhaps ambiguous from reading but this step is optional... it's not like the image is not going to be presented without it. Step 16 is like saying "adjust the image presentation, now that the preferred density-correction dimensions are set" |
| 10:59 | <Noam Rosenthal> | Steps 8, 9, 10: it was a mitigation to having the density set accidentally due to e.g. EXIF information being copied around. Requiring the dimensions to have positive numbers and only use inches narrows the spectrum for accidental density setting. We can reason about making it more lax, it was a bit of a judgement call at the time |
| 11:01 | <zcorpan> | Noam Rosenthal: so then PixelXDimension and PixelYDimension are also ignored for the early return cases |
| 11:01 | <Noam Rosenthal> | yea they are |
| 11:01 | <Noam Rosenthal> | it's like "this is accidental EXIF information" |
| 11:03 | <zcorpan> | I see, thanks. It's still not clear to me that step 16 is not important, as it was the text in place of this algorithm before. It can be read as "don't render cross-origin images" or "don't render images with bogus EXIF" |
| 11:04 | <zcorpan> | I'll file an issue |
| 11:05 | <Noam Rosenthal> | Agreed, it can&should be made clearer. If you file an issue I'm happy to file a PR |
| 11:08 | <zcorpan> | Noam Rosenthal: https://github.com/whatwg/html/issues/11777 |
| 12:26 | <jjaschke> | Noam Rosenthal: Reviewing #11762 right now, thanks for working on this! I like the idea of passing the upcoming tracker into the event firing mechanism, but now I wonder if we need to keep the upcoming trackers in the global state at all, or if we can get away with just passing them around. That way we're less prone to accidentally breaking global state. The only reason I see currently (I may be wrong though) why we need the upcoming non-traverse API method tracker would be to signal that navigation has aborted (this step in navigate()), but I bet we can transport this information in another fashion (which may even be easier to understand).WDYT? |
| 12:32 | <Noam Rosenthal> | Yea it would be great not to have those tidbits of global state... let me scan it a few times :) |
| 12:33 | <jjaschke> | Oh, also just noticed, the promote upcoming to ongoing algorithm that you inlined is also used in reload(). |
| 12:39 | <Noam Rosenthal> | THat's not that algo, it's "maybe setting", not "promote". There was only one use of "promote" |
| 12:40 | <Noam Rosenthal> | I think navigate can have a return value of whether the navigation was aborted prior to the promotion step |
| 12:45 | <jjaschke> | Ugh, of course. But, shouldn't we do the same we do for navigate (extract the upcoming tracker, then run the abort loop, then pass it back in) for reload? |
| 12:47 | <Noam Rosenthal> | No need. Because we do that at the beginning of https://html.spec.whatwg.org/#fire-a-push/replace/reload-navigate-event before we abort the previous one. It's only needed in the regular navigate() because it's also aborted earlier |
| 12:47 | <Noam Rosenthal> | Oh sorry we don't. will update |
| 12:53 | <jjaschke> | Noam Rosenthal : I see two ways we could follow. One would be to remove upcoming non-traverse API method tracker altogether and just pass it around (if that works, I'd prefer that). Or we keep it, but then I think it should be enough to move it out of the global state here, then I don't think we need the change in the navigate algorithm (and pass the upcoming non-traverse tracker into the event firing algorithm) |
| 12:55 | <Noam Rosenthal> | That's not enough because navigateerror can also be called from https://html.spec.whatwg.org/#beginning-navigation:set-the-ongoing-navigation |
| 12:56 | <jjaschke> | Thanks. Then I'd hope we can go for variant one and remove it from the global state whatsoever. |
| 13:02 | <Noam Rosenthal> | https://github.com/whatwg/html/pull/11779 |
| 13:44 | <Noam Rosenthal> | OK I think I got it (in the new PR). I moved that state to be a boolean "has been promoted" on the method tracker. then we don't have to mess so much with navigate |
| 13:46 | <Noam Rosenthal> | this "upcoming method tracker" bit is only for navigate() and reload() calls to ensure that those calls return value is in sync with navigation.transition etc. so I think this new design is cleaner... it's either created early in those calls and passed as an argument, or later on for use of navigation.transition and the rest. |
| 13:57 | <jjaschke> | (threading this for less noise) Noam Rosenthal : Why do we still need the upcoming non-traverse API method tracker as an object on the navigation object? |
| 13:58 | <Noam Rosenthal> | sorry, PR is not finished I realized |
| 14:45 | <Noam Rosenthal> | OK I think it's ready |
| 14:45 | <jjaschke> | looking |
| 14:49 | <Noam Rosenthal> | I think there is a missing thing there (and probably in existing implementation) - throwing an error when reload() fails. e.g. if your reload in an iframe aborts an existing navigation and the navigateerror event made the parent remove the iframe. this would currently return a defunct method tracker |
| 14:58 | <Noam Rosenthal> | in general I think this PR doesn't handle well the case that the document becomes inactive in the navigateerror even during a reload/navigate, because of the early "upcoming" check. |
| 15:03 | <Noam Rosenthal> | (taking a look, I think it was OK before because we were making those checks a bit late) |
| 15:04 | <jjaschke> | Shouldn't we just hit the document-is-active check in commit, which would then fail, and then we bail? |
| 15:06 | <Noam Rosenthal> | yea, but navigate() and reload() think everything is OK, and return a {committed, finished} promise dictionary that's never resolved |
| 15:11 | <Noam Rosenthal> | (nobody rejects those promises because they're only "upcoming") |
| 15:25 | <jjaschke> | Where would they have been rejected before? Wouldn't the old implementation also go into the commit step and return early? |
| 15:27 | <Noam Rosenthal> | yea but they were created after the abort phase (which is the original issue) |
| 15:28 | <Noam Rosenthal> | so the "upcoming" and the "ongoing" trackers would be the same, and the "early error" result (rejected promises) would be returned |
| 15:29 | <jjaschke> | What if we did another document-is-active check at after the abort loop? |
| 15:29 | <Noam Rosenthal> | Uploaded a new revision |
| 15:30 | <Noam Rosenthal> | there is another active check at "fire the inner navigate event", but it asserts if "upcoming method tracker" was passed in |
| 15:31 | <Noam Rosenthal> | I removed this assert. Instead, when we are about to return from reload/navigate, we check that the tracker was indeed promoted. Otherwise we return the rejected thingy |
| 15:35 | <jjaschke> | But wouldn't we set the promoted flag even if the document is not active anymore? |
| 15:35 | <Noam Rosenthal> | nope |
| 15:36 | <Noam Rosenthal> | https://html.spec.whatwg.org/multipage/nav-history-apis.html#navigate-event-firing%3Ahas-entries-and-events-disabled |
| 15:37 | <Noam Rosenthal> | inactive documents will get "has entries and events disabled". So "inner navigate" would assert (or return early if I remove the assert) |
| 15:37 | <jjaschke> | Ohh, forgot about has entries and events disabled |
| 15:38 | <jjaschke> | We really need a spec DSL so that we can run step by step and see state changes and such |
| 15:39 | <Noam Rosenthal> | hehe yea, my brain gets into labyrinth mode when I have to do this stuff. |
| 15:40 | <jjaschke> | Should firing the event still return true in that case? |
| 15:42 | <Noam Rosenthal> | no |
| 15:42 | <Noam Rosenthal> | it should return rejected promises |
| 15:43 | <jjaschke> | I mean this |
| 15:44 | <jjaschke> | Which would mean we don't early return here |
| 15:45 | <jjaschke> | (I don't know off the top of my head what happens after that) |
| 15:46 | <Noam Rosenthal> | yea it should be true. otherwise about:blank and opaque origins couldn't navigate at all |
| 15:47 | <Noam Rosenthal> | but perhaps it should return false specifically when aborting made the document inactive, or there should be a check for that at the call site |
| 15:48 | <Noam Rosenthal> | I guess we can return false if apiMethodTracker is not null |
| 15:49 | <jjaschke> | I think that makes sense |
| 15:50 | <jjaschke> | Let me try to implement these changes real quick to see if it breaks anything :) |
| 16:29 | <mfreed> | Hi all, just a friendly reminder to post any discussion topics for this Thursday's joint CSSWG/WHATWG/OpenUI task force meeting to the meeting agenda issue: https://github.com/whatwg/html/issues/11740 |
| 17:08 | <jjaschke> | Noam Rosenthal: Looking quite good, I only get a few fails, and I'm not sure if that's just a bug in my changes. This test fails because the promise gets rejected instead of just never settling. I think that is because we don't fire the push/replace event. Does that make sense? |
| 17:09 | <jjaschke> | (EOD for me) |
| 17:41 | <Noam Rosenthal> | the assert is wrong. This is actually a bug that this PR fixes |
| 17:41 | <Noam Rosenthal> | "has entries and events disabled" is false for the initial about:blank, so navigate and reload should return rejected promises |
| 17:42 | <Noam Rosenthal> | I think I'm going to make one change which is to move the "return false" thing to right after the abort loop |
| 18:48 | <Noam Rosenthal> | sorry it's a lie. The test is right; When entries/events are disabled the promises should be muted (never-settled) |
| 19:21 | <Noam Rosenthal> | Revised.
|
| 19:21 | <Noam Rosenthal> | This should fix the broken test |