Skip to content

Comments

Don't attempt to format a tuple with "%s"#11050

Merged
feerrenrut merged 1 commit intonvaccess:betafrom
agashlin:master
Apr 24, 2020
Merged

Don't attempt to format a tuple with "%s"#11050
feerrenrut merged 1 commit intonvaccess:betafrom
agashlin:master

Conversation

@agashlin
Copy link
Contributor

This fixes #10736, errors in mousemove events over some MSHTML elements
(such as buttons).

Link to issue number:

fixes #10736

Summary of the issue:

mouse move over buttons doesn't read the button text in Internet Explorer 8 MSHTML, fails with a logged TypeError exception.

Description of how this pull request fixes the issue:

In order to keep the % operator from trying to format each element of the Point namedtuple, wrap the position in a one element tuple. This way the right exception gets thrown and caught. I went into more detail in #10736 (comment)

Testing performed:

I confirmed that this allowed the button to be read in the embedded MSHTML instance in the Firefox stub installer. Build 1 here can be used to demonstrate the issue, though it has other problems: the Re-install button does not read out when moused over unless this fix is made.

Known issues with pull request:

There are possibly other similar issues, but I didn't go looking for them.

Change log entry:

Bug fixes
Fix mouse tracking for some MSHTML elements

@josephsl
Copy link
Contributor

josephsl commented Apr 24, 2020 via email

@AppVeyorBot
Copy link

See test results for failed build of commit c35370856f

@agashlin
Copy link
Contributor Author

Hi, can you verify that this works in IE11, as IE8 is too old at this time? Thanks.

Yes, I confirmed that this issue is also present in IE11, and this fixes it.

source/NVDAObjects/IAccessible/MSHTML.py:333:44: E228 missing whitespace around modulo operator

I'll fix this and try running a local flake8.

@AppVeyorBot
Copy link

See test results for failed build of commit 9b3f4a82d7

@agashlin
Copy link
Contributor Author

I suspect this system test failure is unrelated to my change, as that test uses Chrome and this only touches MSHTML. Is it possible to re-run the system test?

@josephsl
Copy link
Contributor

josephsl commented Apr 24, 2020 via email

@feerrenrut
Copy link
Contributor

I suspect this system test failure is unrelated to my change

@agashlin Yes we are getting intermittent failures that I haven't identified. I have restarted the build for you.

I think a better solution is reverting system tests commit

Rather than revert, I think we should just disable until we have time to look at what is causing the failure. I'll look at this today.

Copy link
Contributor

@feerrenrut feerrenrut left a comment

Choose a reason for hiding this comment

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

Thanks for the investigation and fix @agashlin

Another approach might be to use an f-string eg: f"position: {position}"

@feerrenrut
Copy link
Contributor

Since this is a small, low risk change, I think it should go into the next release which is currently stalling anyway. I'll change the target to beta

@feerrenrut feerrenrut changed the base branch from master to beta April 24, 2020 08:40
@feerrenrut feerrenrut added bug p3 https://github.com/nvaccess/nvda/blob/master/projectDocs/issues/triage.md#priority labels Apr 24, 2020
@feerrenrut
Copy link
Contributor

But the PR will need to be rebased onto beta. I'm happy to do that.

This fixes #10736, errors in mousemove events over some MSHTML elements
(such as buttons).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug p3 https://github.com/nvaccess/nvda/blob/master/projectDocs/issues/triage.md#priority

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Many errors in log when moving the mouse in Internet explorer

5 participants