Bug 247445
Summary: | Please remove a desmos.com quirks entry | ||
---|---|---|---|
Product: | WebKit | Reporter: | Jason Merrill <jwmerrill> |
Component: | UI Events | Assignee: | Karl Dubost <karlcow> |
Status: | RESOLVED FIXED | ||
Severity: | Normal | CC: | karlcow, koivisto, webkit-bug-importer, zalan |
Priority: | P2 | Keywords: | InRadar |
Version: | WebKit Nightly Build | ||
Hardware: | Unspecified | ||
OS: | Unspecified | ||
See Also: |
https://bugs.webkit.org/show_bug.cgi?id=199508 https://bugs.webkit.org/show_bug.cgi?id=196830 https://bugs.webkit.org/show_bug.cgi?id=197652 https://bugs.webkit.org/show_bug.cgi?id=247555 |
Jason Merrill
desmos.com is currently targeted in Quirks.cpp: https://github.com/WebKit/WebKit/blob/a6c922ea6f0526752da82adfad89afd1ef26268a/Source/WebCore/page/Quirks.cpp#L442-L443
The quirk test requires the url to include `/calculator/`, but the graphing calculator start page does not include a trailing slash (it is https://www.desmos.com/calculator).
URLs for saved graphs do include `/calculator/` in the path (example: https://www.desmos.com/calculator/2rnqgoa6a4).
Desmos recently ran into a bug that was only showing up in saved graphs, and not for the blank calculator. We eventually traced the difference back to this quirk.
I don't believe this quirk is required anywhere on Desmos anymore, and in any case, it was not being applied to the most commonly used calculator page. Please remove it.
Attachments | ||
---|---|---|
Add attachment proposed patch, testcase, etc. |
Jason Merrill
As an aside, is there anything sites can do to opt out of certain quirks? Even if this quirk is removed, I'm wondering if there's anything Desmos can do to make sure it does not affect us on previous releases of the browser.
Karl Dubost
Hi Jason,
First of all, thanks a lot for reporting the issue.
Could you give a bit more information on
1. the steps to reproduce the issue
2. which device this is happening.
3. which version of the OS and Safari. :) (I suspect iOS or iPadOS)
Digging through the code
There are two quirks currently
https://github.com/WebKit/WebKit/blob/4718faa509f327097a30847ee21ede844aff6670/Source/WebCore/page/Quirks.cpp#L541-L543
```cpp
std::optional<Event::IsCancelable> Quirks::simulatedMouseEventTypeForTarget(EventTarget* target) const
{
if (!shouldDispatchSimulatedMouseEvents(target))
return { };
// cut for brevity
auto host = m_document->topDocument().url().host();
if (equalLettersIgnoringASCIICase(host, "desmos.com"_s) || host.endsWithIgnoringASCIICase(".desmos.com"_s))
return Event::IsCancelable::No;
// cut for brevity
return Event::IsCancelable::Yes;
}
```
and
https://github.com/WebKit/WebKit/blob/4718faa509f327097a30847ee21ede844aff6670/Source/WebCore/page/Quirks.cpp#L442-L443
```cpp
bool Quirks::shouldDispatchSimulatedMouseEvents(const EventTarget* target) const
{
if (DeprecatedGlobalSettings::mouseEventsSimulationEnabled())
return true;
if (!needsQuirks())
return false;
auto doShouldDispatchChecks = [this] () -> ShouldDispatchSimulatedMouseEvents {
auto* loader = m_document->loader();
if (!loader || loader->simulatedMouseEventsDispatchPolicy() != SimulatedMouseEventsDispatchPolicy::Allow)
return ShouldDispatchSimulatedMouseEvents::No;
// cut for brevity
auto& url = m_document->topDocument().url();
auto host = url.host().convertToASCIILowercase();
// cut for brevity
if ((host == "desmos.com"_s || host.endsWith(".desmos.com"_s)) && startsWithLettersIgnoringASCIICase(url.path(), "/calculator/"_s))
return ShouldDispatchSimulatedMouseEvents::Yes;
// cut for brevity
return ShouldDispatchSimulatedMouseEvents::No;
};
// cut for brevity
ASSERT_NOT_REACHED();
return false;
}
```
## First Quirk
The first one was started on 2019-04-11
Bug 196830
<rdar://problem/49124313>
https://github.com/WebKit/WebKit/commit/a2839dea43fe5b86b6d5ec6132137dd462ed7094
and then adjusted on 2019-05-07
Bug 197652
<rdar://problem/47068176>
https://github.com/WebKit/WebKit/commit/721679dc53a16b6ba9a05b5b4f5b993f9f4cf991
This is enabled only if Touch events are active.
https://github.com/WebKit/WebKit/blob/4718faa509f327097a30847ee21ede844aff6670/Source/WebCore/page/Quirks.h#L68-L69
The initial steps to reproduce were:
Steps:
1. Visit https://www.desmos.com/calculator
2. Touch the graph screen to move the graph around
3. Touch the formula field
4. Type keys on soft or HW keyboard
Expected Results:
Characters are entered into formula field
Actual Results:
No characters are entered into formula field
## Second Quirk
The second one was started on 2019-07-04
Bug 199508
<rdar://problem/50925173>
https://github.com/WebKit/WebKit/commit/9d226cb633d0dd7379f5de1bc6c245e8f583cc5c
Steps
1. Load https://www.desmos.com/calculator/vymzvpqp3k or any sheet with more formulas than will fit on screen.
2. Swipe to scroll formula
Results:
Formula region does not scroll.
Karl Dubost
Jason,
## Disabling Site Quirks
Safari Desktop has a "Disable Site Specific Hacks" in the Develop menu, but not Safari on iPad it seems. So indeed that makes it a bit harder to test. I will test in a build without the Quirks lines to double check.
Maybe I missed it, but just in case to not forget. Bug 247555
## Opting Out of Quirks for a Website
> Is there anything sites can do to opt out of certain quirks? Even if this quirk is removed, I'm wondering if there's anything Desmos can do to make sure it does not affect us on previous releases of the browser.
This is a very interesting question.
The current answer is no. But the idea of a kill switch for a site interventions for older versions of the browsers is interesting.
If such feature existed, what would be the way to trigger it on your side?
Karl Dubost
Jason,
to deactivate the Quirks, you could connect the external device to a Mac and with the Web Inspector deactivate it.
See https://webkit.org/web-inspector/device-settings/
Jason Merrill
(In reply to Karl Dubost from comment #2)
My eventual goal is to have all site specific quirks for desmos.com removed. Desmos licenses our calculator products to partners to embed in other domains, and this is a very important part of our business. Any place that the calculator does not function correctly outside of desmos.com is a problem for us and our partners.
But I would like to focus this issue on what you describe as "Quirk 2" because I think that one may be easiest to deal with.
> Could you give a bit more information on
>
> 1. the steps to reproduce the issue
This is tricky because I think Desmos is currently set up to function correctly either with or without Quirk 1 in effect. We have observed that it changes which mouse and touch events are fired, but there's no user facing way to observe this that we know of. You'd have to add extra event listeners with logging to easily see this.
> 2. which device this is happening.
> 3. which version of the OS and Safari. :) (I suspect iOS or iPadOS)
We have observed the effects of this quirk on two different iPads, one running iPadOS 15.6.1, and another running iPadOS 16.
>
> Digging through the code
>
> There are two quirks currently
> https://github.com/WebKit/WebKit/blob/
> 4718faa509f327097a30847ee21ede844aff6670/Source/WebCore/page/Quirks.cpp#L541-
> L543
>
[snip]
>
> and
> https://github.com/WebKit/WebKit/blob/
> 4718faa509f327097a30847ee21ede844aff6670/Source/WebCore/page/Quirks.cpp#L442-
> L443
>
> ```cpp
> bool Quirks::shouldDispatchSimulatedMouseEvents(const EventTarget* target)
> const
> {
> if (DeprecatedGlobalSettings::mouseEventsSimulationEnabled())
> return true;
>
> if (!needsQuirks())
> return false;
>
> auto doShouldDispatchChecks = [this] () ->
> ShouldDispatchSimulatedMouseEvents {
> auto* loader = m_document->loader();
> if (!loader || loader->simulatedMouseEventsDispatchPolicy() !=
> SimulatedMouseEventsDispatchPolicy::Allow)
> return ShouldDispatchSimulatedMouseEvents::No;
>
> // cut for brevity
>
> auto& url = m_document->topDocument().url();
> auto host = url.host().convertToASCIILowercase();
>
> // cut for brevity
>
> if ((host == "desmos.com"_s || host.endsWith(".desmos.com"_s)) &&
> startsWithLettersIgnoringASCIICase(url.path(), "/calculator/"_s))
> return ShouldDispatchSimulatedMouseEvents::Yes;
>
> // cut for brevity
>
> return ShouldDispatchSimulatedMouseEvents::No;
> };
>
> // cut for brevity
>
> ASSERT_NOT_REACHED();
> return false;
> }
> ```
I think there's an easy case to make that this quirk should be removed: it only targets paths starting with "/calculator/" (note the trailing slash), and so it is not targeting the most common URL for using the calculator (https://www.desmos.com/calculator) (note, no trailing slash).
>
> ## First Quirk
>
[snip to instead focus on the second quirk]
>
> ## Second Quirk
>
> The second one was started on 2019-07-04
> Bug 199508
> <rdar://problem/50925173>
> https://github.com/WebKit/WebKit/commit/
> 9d226cb633d0dd7379f5de1bc6c245e8f583cc5c
>
> Steps
> 1. Load https://www.desmos.com/calculator/vymzvpqp3k or any sheet with more
> formulas than will fit on screen.
> 2. Swipe to scroll formula
>
> Results:
> Formula region does not scroll.
Following these original reproduction steps, I think we can see that Quirk 2 is no longer necessary by observing that is not applied to https://www.desmos.com/calculator but is applied to saved graph URLs such as https://www.desmos.com/calculator/utywo6ohyn.
You can try
1. visiting https://www.desmos.com/calculator on an iPad running iPadOS 15 or 16 (and likely other versions), where the quirk is not applied
2. Entering many formulas, say 20 formulas with the first 20 integers
3. Observing that swiping to scroll the list of formulas works correctly
vs.
1. visiting https://www.desmos.com/calculator/utywo6ohyn (a saved graph where the quirk is applied)
2. Observing that the list of formulas is also scrollable by swiping here.
Karl Dubost
Pull request: https://github.com/WebKit/WebKit/pull/6332
EWS
Committed 256558@main (8e0e88f4f3d2): <https://commits.webkit.org/256558@main>
Reviewed commits have been landed. Closing PR #6332 and removing active labels.
Karl Dubost
Jason,
One of the Quirks has been deactivated on WebKit.
Jason Merrill
That's great. Thank you.