[Affected versions]: Nightly 61.0a1 [Affected Platforms]: Ubuntu 16.04 x64, Windows 10 x64 [Prerequisites]: - Have the latest try build 61.0a1 from (2018-03-27) installed - Accessibility is enabled and accessibility tab is opened - Responsive Design mode is enabled [Steps to reproduce]: 1. Select any of the listed devices resolution 2. Click the "Pick accessible objects from the page" button 3. Hover over any element from the page's content [Expected result]: The element inspected with Accessibility should be highlighted with a purple transparent overlay. [Actual result]: The purple highlight overlay is misplaced or it is not displayed at all.

(In reply to Timea Babos from comment #0) > Created attachment 8964524 [details] > Screenshot of the issue - 2018 -04-03 > > [Affected versions]: > Nightly 61.0a1 > > [Affected Platforms]: > Ubuntu 16.04 x64, Windows 10 x64 > > [Prerequisites]: > - Have the latest try build 61.0a1 from (2018-03-27) installed > - Accessibility is enabled and accessibility tab is opened > - Responsive Design mode is enabled > > [Steps to reproduce]: > 1. Select any of the listed devices resolution > 2. Click the "Pick accessible objects from the page" button > 3. Hover over any element from the page's content > > [Expected result]: > The element inspected with Accessibility should be highlighted with a purple > transparent overlay. > > [Actual result]: > The purple highlight overlay is misplaced or it is not displayed at all. Hi Timea, Could you confirm that this only happens when the DPR is changed, not the resolution? If that's not the case, could you provide what values you have as defaults and what values do you change to?

Hey Yura, The highlight is placed correctly with the following default values: - No throttling - DPR: 1 - No device selected - None of the options in “Reload when...” are checked The issue occurs if either the DPR is changed to other values (DPR:2; DPR:3) or any of the listed devices resolution is selected, thus the DPR option is automatically changed according to the selected device. If any custom resolution is created by dragging the window and resizing manually, the highlight will be properly displayed. As you said, it occurs only if the DPR option is changed to any possible value, except the default DPR:1. Hope this was helpful, please ask me for any further information you need.

(In reply to Timea Babos from comment #2) > Hey Yura, > > The highlight is placed correctly with the following default values: > - No throttling > - DPR: 1 > - No device selected > - None of the options in “Reload when...” are checked > > The issue occurs if either the DPR is changed to other values (DPR:2; DPR:3) > or any of the listed devices resolution is selected, thus the DPR option is > automatically changed according to the selected device. > If any custom resolution is created by dragging the window and resizing > manually, the highlight will be properly displayed. As you said, it occurs > only if the DPR option is changed to any possible value, except the default > DPR:1. > Hope this was helpful, please ask me for any further information you need. Thanks, very helpful.

Alex for accessible/ review Jimm to take a look at the ipc/ipdl/sync-messages.ini changes

Comment on attachment 8969769 [details] [diff] [review] 1450927 accessible patch Review of attachment 8969769 [details] [diff] [review]: ----------------------------------------------------------------- ::: accessible/generic/Accessible.cpp @@ +693,5 @@ > +Accessible::BoundsInCSSPixels() const > +{ > + nsRect bounds = BoundsInAppUnits(); > + nsPresContext* presContext = mDoc->PresContext(); > + return bounds.ToNearestPixels(presContext->AppUnitsPerCSSPixel()); it makes sense to make these two functions inline and also it can be written a shorter way: BoundsInAppUnits().ToNearestPixels(mDoc->PresContext()->AppUnitsPerCSSPixel()); ::: accessible/generic/Accessible.h @@ +511,5 @@ > > + /** > + * Return boundaries in screen coordinates in app units. > + */ > + virtual nsRect BoundsInAppUnits() const; ah, so no sense to make those inline until you de-vertualize them ::: accessible/html/HTMLListAccessible.cpp @@ +88,5 @@ > return rect; > } > > +nsIntRect > +HTMLLIAccessible::BoundsInCSSPixels() const it'd be nicer to convert Bounds() into BoundsInAppPixels(), since it allows you to avoid code duplication, and also makes you closer to de-vertualizing BoundsInCSSPixels ::: accessible/interfaces/nsIAccessible.idl @@ +28,5 @@ > * nodes in the DOM tree -- such as documents, focusable elements and text. > * Mozilla creates the implementations of nsIAccessible on demand. > * See http://www.mozilla.org/projects/ui/accessibility for more information. > */ > +[scriptable, builtinclass, uuid(ea662d70-4311-11e8-b566-0800200c9a66)] I vaguely recall that we no longer required to change uuids, I can't see how it can harm, but maybe it makes sense to double check that with someobdy who knows @@ +225,5 @@ > nsIArray getRelations(); > > /** > * Return accessible's x and y coordinates relative to the screen and > * accessible's width and height. worth to mention that these are in dev pixels @@ +233,5 @@ > + /** > + * Return accessible's x and y coordinates relative to the screen and > + * accessible's width and height in CSS pixels. > + */ > + void getBoundsInCSSPixels(out long aX, out long aY, out long aWidth, out long aHeight); curious if DOMRect will be a better match ::: accessible/tests/mochitest/bounds/test_list.html @@ +46,5 @@ > "Outside list item y should match to list item element y"); > ok(widthLI > widthLIElm, > "Outside list item width=" + widthLI + " should be greater than list item element width=" + widthLIElm); > + ok(heightLI > heightLIElm, > + "Outside list item height=" + heightLI + " should be greater than list item element height=" + heightLIElm); I presume this is because of new ToNearestPixels, so could you convert these to test the difference is not bigger than 1? ::: accessible/tests/mochitest/layout.js @@ +186,5 @@ > } > > /** > * Return the accessible coordinates and size relative to the screen in device > * pixels. might be nice to adjust the comment to mention this method tests css vs dev boundaries as well @@ +196,5 @@ > accessible.getBounds(x, y, width, height); > + accessible.getBoundsInCSSPixels(xInCSS, yInCSS, widthInCSS, heightInCSS); > + > + const { devicePixelRatio } = window; > + ok(Math.round(x.value / devicePixelRatio) >= xInCSS.value, "X in CSS pixels is calculated correctly"); >= for error of 1 looks scary, so this ceil function might work well: is(Math.ceil(x.value / devicePixelRatio), xInCSS.value, "bla") or Math.trunct might work well. If it doesn't, then ok(Math.abs(x.value / devicePixelRatio - xInCSS.value) <= 1, "bla"); ::: accessible/xul/XULTreeAccessible.cpp @@ +722,5 @@ > return FocusMgr()->FocusedAccessible() == this ? this : nullptr; > } > > nsIntRect > +XULTreeItemAccessibleBase::BoundsRect() const so this one actually should be BoundsInCSSPixels, and also you should provide BoundsInAppPixels for correctness. Same applied to XULGridTree

Comment on attachment 8969770 [details] [diff] [review] 1450927 devtools patch Review of attachment 8969770 [details] [diff] [review]: ----------------------------------------------------------------- I won't be able to build and review this quickly, forwarding to Ryan.

Comment on attachment 8969769 [details] [diff] [review] 1450927 accessible patch canceling review until comments are addressed

Marking Alex for a11y parts. Marking Jed for changes in ipc/ipdl/sync-messages.ini Some context: this method is similar to PDocAccessible::Extents but for CSS pixels.

Comment on attachment 8969770 [details] [diff] [review] 1450927 devtools patch Review of attachment 8969770 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, this looks reasonable to me! :)

Comment on attachment 8970674 [details] [diff] [review] 1450927 accessible patch v2 The sync message change seems reasonable: it's equivalent to adding a parameter to the existing sync message PDocAccessible::Extents.

Rebased a11y changes with Eitan's patch.

Comment on attachment 8970791 [details] [diff] [review] 1450927 accessible patch v3 Review of attachment 8970791 [details] [diff] [review]: ----------------------------------------------------------------- ::: accessible/generic/Accessible.h @@ +517,4 @@ > /** > * Return boundaries in screen coordinates. > */ > virtual nsIntRect Bounds() const; if you remove XULTree/XULTreeGrid overridings, then you can devirtualize the method and make it inline ::: accessible/ipc/other/ProxyAccessible.cpp @@ +1017,5 @@ > +ProxyAccessible::BoundsInCSSPixels() > +{ > + nsIntRect rect; > + Unused << mDoc->SendExtentsInCSSPixels(mID, > + &(rect.x), &(rect.y), nit: do you really need braces around rect.x and so? btw, indentation seems broken ::: accessible/ipc/win/ProxyAccessible.cpp @@ +245,5 @@ > + > + long x; > + long y; > + long width; > + long height; I prefer to initialize local variables @@ +251,5 @@ > + if (FAILED(hr)) { > + return nsIntRect(); > + } > + > + return nsIntRect(x, y, width, height); you could do same as you do in ProxyAccessible and get rid of x, y ... local variables by using nsIntRect directly. ::: accessible/tests/mochitest/layout.js @@ +210,5 @@ > + accessible.getBoundsInCSSPixels(xInCSS, yInCSS, widthInCSS, heightInCSS); > + > + const { devicePixelRatio } = window; > + ok(Math.ceil(x.value / devicePixelRatio) >= xInCSS.value, > + "X in CSS pixels is calculated correctly"); So ceil/trunc didn't work, and the difference can be > 0? ::: accessible/xul/XULTreeAccessible.cpp @@ +754,5 @@ > + return nsIntRect(x, y, width, height); > +} > + > +nsIntRect > +XULTreeItemAccessibleBase::Bounds() const I think it should be ok to not override it, Accessible::Bounds() will work as long as you implement BoundsInAppUnits()

Pushed by yura.zenevich@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/d7dd8b0d2473 add getBoundsInCSSPixels XPCOM method. r=surkov r=jld https://hg.mozilla.org/integration/mozilla-inbound/rev/ca285aed3926 use new getBoundsInCSSPixels method for accessible bounds in accessible highlighter. r=jryans

Backout by toros@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/89bc67f5eaaf Backed out 2 changesets for failing on ProxyAccessible.cpp(247) on a CLOSED TREE

https://hg.mozilla.org/integration/mozilla-inbound/rev/89bc67f5eaaf6348c102e2b0aa6152b87b9c99cc https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=ca285aed3926966562f9444b912eb24d757d2657 https://treeherder.mozilla.org/logviewer.html#?job_id=175551691&repo=mozilla-inbound&lineNumber=26540 16:13:38 INFO - mozmake.EXE[4]: Leaving directory 'z:/build/build/src/obj-firefox/toolkit/components/find' 16:13:38 INFO - mozmake.EXE[4]: Entering directory 'z:/build/build/src/obj-firefox/toolkit/components/find' 16:13:38 INFO - toolkit/components/find 16:13:38 INFO - mozmake.EXE[4]: Leaving directory 'z:/build/build/src/obj-firefox/toolkit/components/find' 16:13:38 INFO - mozmake.EXE[4]: Entering directory 'z:/build/build/src/obj-firefox/toolkit/components/find' 16:13:38 INFO - mozmake.EXE[4]: Leaving directory 'z:/build/build/src/obj-firefox/toolkit/components/find' 16:13:38 INFO - mozmake.EXE[4]: Entering directory 'z:/build/build/src/obj-firefox/accessible/ipc/win' 16:13:38 INFO - z:/build/build/src/sccache2/sccache.exe z:/build/build/src/vs2017_15.6.6/VC/bin/Hostx64/x64/cl.exe -FoProxyAccessible.obj -c -Iz:/build/build/src/obj-firefox/dist/stl_wrappers -DNDEBUG=1 -DTRIMMED=1 -DWIN32_LEAN_AND_MEAN -D_WIN32 -DWIN32 -D_CRT_RAND_S -DCERT_CHAIN_PARA_HAS_EXTRA_FIELDS -DOS_WIN=1 -D_UNICODE -DCHROMIUM_BUILD -DU_STATIC_IMPLEMENTATION -DUNICODE -D_WINDOWS -D_SECURE_ATL -DCOMPILER_MSVC -DSTATIC_EXPORTABLE_JS_API -DMOZ_HAS_MOZGLUE -DMOZILLA_INTERNAL_API -DIMPL_LIBXUL -Iz:/build/build/src/accessible/ipc/win -Iz:/build/build/src/obj-firefox/accessible/ipc/win -Iz:/build/build/src/accessible/base -Iz:/build/build/src/accessible/generic -Iz:/build/build/src/accessible/windows/ia2 -Iz:/build/build/src/accessible/windows/msaa -Iz:/build/build/src/accessible/xpcom -Iz:/build/build/src/obj-firefox/ipc/ipdl/_ipdlheaders -Iz:/build/build/src/ipc/chromium/src -Iz:/build/build/src/ipc/glue -Iz:/build/build/src/obj-firefox/dist/include -Iz:/build/build/src/obj-firefox/dist/include/nspr -Iz:/build/build/src/obj-firefox/dist/include/nss -MD -FI z:/build/build/src/obj-firefox/mozilla-config.h -DMOZILLA_CLIENT -utf-8 -TP -nologo -w15038 -wd5026 -wd5027 -Zc:sizedDealloc- -wd4091 -wd4577 -D_HAS_EXCEPTIONS=0 -W3 -Gy -Zc:inline -Gw -wd4251 -wd4244 -wd4267 -wd4800 -wd4595 -we4553 -D_SILENCE_TR1_NAMESPACE_DEPRECATION_WARNING -GR- -Z7 -O1 -Oi -Oy- -WX -deps.deps/ProxyAccessible.obj.pp z:/build/build/src/accessible/ipc/win/ProxyAccessible.cpp 16:13:38 INFO - ProxyAccessible.cpp 16:13:38 INFO - z:/build/build/src/accessible/ipc/win/ProxyAccessible.cpp(247): error C2664: 'HRESULT IGeckoCustom::get_boundsInCSSPixels(long *,long *,long *,long *)': cannot convert argument 1 from 'int32_t *' to 'long *' 16:13:38 INFO - z:/build/build/src/accessible/ipc/win/ProxyAccessible.cpp(247): note: Types pointed to are unrelated; conversion requires reinterpret_cast, C-style cast or function-style cast 16:13:38 INFO - z:/build/build/src/config/rules.mk:1024: recipe for target 'ProxyAccessible.obj' failed 16:13:38 INFO - mozmake.EXE[4]: *** [ProxyAccessible.obj] Error 2 16:13:38 INFO - mozmake.EXE[4]: Leaving directory 'z:/build/build/src/obj-firefox/accessible/ipc/win' 16:13:38 INFO - z:/build/build/src/config/recurse.mk:73: recipe for target 'accessible/ipc/win/target' failed 16:13:38 INFO - mozmake.EXE[3]: *** [accessible/ipc/win/target] Error 2 16:13:38 INFO - mozmake.EXE[3]: *** Waiting for unfinished jobs.... 16:13:38 INFO - mozmake.EXE[4]: Entering directory 'z:/build/build/src/obj-firefox/toolkit/components/find' 16:13:38 INFO - mozmake.EXE[4]: Leaving directory 'z:/build/build/src/obj-firefox/toolkit/components/find' 16:13:38 INFO - mozmake.EXE[4]: Entering directory 'z:/build/build/src/obj-firefox/toolkit/components/find' 16:13:38 INFO - mozmake.EXE[4]: Leaving directory 'z:/build/build/src/obj-firefox/toolkit/components/find' 16:13:39 INFO - mozmake.EXE[4]: Entering directory 'z:/build/build/src/obj-firefox/accessible/xul'

Pushed by yura.zenevich@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/b58c8b754ce2 add getBoundsInCSSPixels XPCOM method. r=surkov r=jld https://hg.mozilla.org/integration/mozilla-inbound/rev/1c0476f1f505 use new getBoundsInCSSPixels method for accessible bounds in accessible highlighter. r=jryans

Should be fixed now ...

https://hg.mozilla.org/mozilla-central/rev/b58c8b754ce2 https://hg.mozilla.org/mozilla-central/rev/1c0476f1f505

On the latest Firefox Nightly (Build ID: 20180426100055) the issue cannot be reproduced on Mac OS X 10.12, Windows 10 x64 and Ubuntu 16.04 x64.