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

The link to 'focus' is not appropriate #282

Closed
Honry opened this issue Aug 7, 2019 · 20 comments · Fixed by #292
Closed

The link to 'focus' is not appropriate #282

Honry opened this issue Aug 7, 2019 · 20 comments · Fixed by #292
Labels
Origin Trial Issues that would be good to solve before OT

Comments

@Honry
Copy link
Contributor

Honry commented Aug 7, 2019

The "focus" used in the spec links to https://html.spec.whatwg.org/multipage/interaction.html#focused which only applies to elements, rather than the web pages or browsing contexts.
e.g.

Reading an NFC tag containing other than Web NFC message, when a web page using the Web NFC API is open and in focus.

We need to find an another way to indicate the focus, maybe it could be something like:

when a web page using the Web NFC API is open and contains an element which has gained focus.

Thoughts?

@kenchris
Copy link
Contributor

kenchris commented Aug 7, 2019

Did you check what we did in wakelock spec?

@Honry
Copy link
Contributor Author

Honry commented Aug 8, 2019

Wakelock uses "fully active":

When the user agent determines that a responsible document of the current settings object is no longer fully active, it must run these steps:

While generic sensor uses "currently focused area of a top-level browsing context":

Currently focused area belongs to a document whose origin is same origin-domain with the origin of the given active document.

@kenchris
Copy link
Contributor

kenchris commented Aug 8, 2019

So area means that is refers to the viewport? That doesn't really make sense here, @anssiko

@kenchris
Copy link
Contributor

kenchris commented Aug 8, 2019

What we really want is that the web site is active and visible to the user. I don't really see why we mix this with element focus

@kenchris
Copy link
Contributor

kenchris commented Aug 8, 2019

image

Whether we are using "active" or "fully active" depends on whether we support the feature in iframes etc, which I don't think we should do (hard to convey to users and what is the use-case?)

Doesn't the visibility check make the focused area check pointless? @anssiko

@kenchris
Copy link
Contributor

kenchris commented Aug 8, 2019

Btw, I like the way this was specced in Generic Sensors, we should do the same, very clear

@anssiko
Copy link
Member

anssiko commented Aug 8, 2019

Both checks are needed to mitigate skimming attacks. A window can be both visible and not currently focused at the same time in traditional windowing systems.

@riju
Copy link
Collaborator

riju commented Aug 8, 2019

From sensor impl point of view : readings are only available for active documents whose origin is same origin-domain with the currently focused frame (LocalFrame)

focused_frame = GetPage()->GetFocusController().FocusedFrame()

@Honry
Copy link
Contributor Author

Honry commented Aug 8, 2019

BTW, should Web NFC API be allowed in an iframe? Currently it is not allowed in Chromium implementation, but I think it should be, and we may need integrate the Feature Policy API. Thoughts?

@riju
Copy link
Collaborator

riju commented Aug 8, 2019

I don't think we need to support webNFC on iframes right now, unless some specific use case comes up. WebNFC implementation is now following the same way as sensors and I don't see a reason to change that atleast for the time being.

@Honry
Copy link
Contributor Author

Honry commented Aug 8, 2019

So a get focused iframe(same origin-domain) is able to expose Web NFC API, right? Same as the sensor implementation.

@riju
Copy link
Collaborator

riju commented Aug 8, 2019

Sorry, was not clear,
The Security Policies from webNFC second bullet point, states that it should be exposed to the top-level browsing context. I guess it translates to -
!To<Document>(context)->GetFrame()->IsMainFrame())
which should exclude iframes , right ?

Which is already checked in NFCProxy

So yes, in that respect a bit different from sensors, which does allow access from iframes with Feature Policy and Focus Areas.

@kenchris
Copy link
Contributor

kenchris commented Aug 8, 2019

Yeah, we can consider supporting the same in the future, but for now we can do simpler.

@anssiko there is a difference between the frame having focus and a certain element.

@anssiko
Copy link
Member

anssiko commented Aug 8, 2019

@kenchris
Copy link
Contributor

kenchris commented Aug 9, 2019

The term focusable area is used to refer to regions of the interface that can become the target of keyboard input. Focusable areas can be elements, parts of elements, or other regions managed by the user agent.

Yes, this sounds like element focus and not about whether the containing browser window has focus. Web NFC and Generic Sensors are not elements or contained by elements so it feels wrong. Also on mobile you often don't have keyboard focus because the keyboard is visible if you do

Honry added a commit to Honry/web-nfc that referenced this issue Aug 12, 2019
kenchris pushed a commit that referenced this issue Aug 14, 2019
@kenchris kenchris reopened this Aug 21, 2019
@riju riju added the Origin Trial Issues that would be good to solve before OT label Aug 23, 2019
@leonhsl
Copy link
Contributor

leonhsl commented Sep 5, 2019

Do we have other remaining tasks for this issue? Looks like the spec already got refined and our impl is also fine with it.

@kenchris
Copy link
Contributor

Conclusion from TPAC meeting is that we drop relying on focus and adopt prose similar to wake lock, meaning that we abort push/scan when we loose visibility

@kenchris
Copy link
Contributor

Text from wakelock:

image

This is what we have in Web NFC:

image

It would be better to turn that into algorithms instead of just text. We should also consider just aborting the push when we lose visibility.

@kenchris
Copy link
Contributor

@zolkis do you agree with this, then I can probably work on this tomorrow?

@zolkis
Copy link
Contributor

zolkis commented Sep 30, 2019

Certainly. Let me finish the current PR, let's review/merge and then do this tomorrow.

kenchris added a commit to kenchris/web-nfc that referenced this issue Oct 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Origin Trial Issues that would be good to solve before OT
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants