Hyperlink: resize underline and focus rectangles to text width; ignore tap outside of text area #4320
Conversation
|
I think there are new test failures (window tests on the ubuntu CI) is it possible the hyperlink is used in them and this change is impacting the outcomes? |
That is certainly possible and looking further at the test log I think that is the case. I'll try to resolve this later in the evening. |
|
It seems like this has uncovered a platform difference between Ubuntu and the other desktop OSes - I can see from adding debug logs that MouseIn is being called on the Hyperlink widget, which then sets the hovered flag to true so that the Cursor() function will return Pointer cursor on the next invocation - but it seems like it calls Cursor first, and only once, where on Mac/Windows, it either must call cursor twice, or calls it after MouseIn. I think we can either remove Hyperlink from that test, or else correct for this platform difference in the driver The behavior on Ubuntu for an actual, visible window with a hyperlink manually being hovered is correct Edit: The order of operations when a mouse move happens is indeed to a) ask the widget for its cursor type, and b) inform the widget of the mouse movement if it is mousable. And - tested on MacOS - if you move the mouse just one pixel to enter the focus area of the hyperlink, the hyperlink underlines itself - since the text area is hovered - but the cursor does not change (until the next mouse movement). So I'm surprised the unit test passed at all on Mac or Windows, but it does seem that switching the order of operations would be correct, since widgets should be first informed about the mouse position before they are asked what cursor should be drawn. I'll hold off on making that change @andydotxyz until you OK the plan. I also had initially forgotten to handle text alignment, and those changes are pushed now. |
|
I cannot re-run them but it seems that the cursor tests on Ubuntu are all failing? |
|
@dweymouth it looks like tests are failing on Ubuntu due to some Cursor issues. |
|
From what I could tell the tests were failing because we actually have "incorrect" behavior - in that the widget is asked for its cursor type before the mouse move is processed, so if the mouse moves in one event onto the hyperlink's tappable area, the cursor doesn't actually change until the next mouse event. I am on vacation right now so probably won't really look at this until next week, so if someone wanted to take it over, go for it! But the problem was that the Cursor function is called synchronously while mouse events were placed in an event queue before being sent to the widget. So the challenge is to find out a way to ensure Cursorable widgets always have their Cursor function called after each mouse movement, so if they need to update their cursor type in response to the mouse movement, they can do so. |
|
It would be nice to get this landed. Just a friendly reminder so to say :) |
|
I think we need @andydotxyz to make an executive decision here -
|
|
I think the first of the two may be ok assuming the test fixes are pretty simple. |
Description:
Constrain the focus rectangle, underline, and tappable area to the part of the hyperlink widget that is actually displaying text, if the widget itself is sized larger than the text display area.
Fixes #3528
Checklist:
Where applicable: