Clickable area of a text object is much larger than the object itself (rev >= 12488)

[Ryan Lerch]

Steps to recreate:
1. Create a normal (unflowed) text object
2. Resize the text larger using the select/resize handles
3. Move the mouse above the text object to see the bounds of the selectable area.

Attached is a screencast of the issue. Notice the cursor not change when it is moved above the text object, but out of the normal bounds.

[su_v (suv-lp)]

Reproduced with r12712 on OS X 10.7.5

Based on tests with archived builds:
- not reproduced with rev <= 12487
- reproduced with rev >= 12488
this regression was introduced with the merge of the EMF/WMF branch:
<http://bazaar.launchpad.net/~inkscape.dev/inkscape/trunk/changes/12488>

[jazzynico (jazzynico)]

According to the comments in the code (see https://bazaar.launchpad.net/~inkscape.dev/inkscape/trunk/view/head:/src/display/drawing-text.cpp#L78), it was done purposely:

----
 /* Make a bounding box that is a little taller and lower (currently 10% extra) than the font's drawing box. Extra space is
    to hold overline or underline, if present. All characters in a font use the same ascent and descent,
    but different widths. This lets leading and trailing spaces have text decorations. If it is not done
    the bounding box is limited to the box surrounding the drawn parts of visible glyphs only, and draws outside are ignored.
 */
----

IMHO, such a change should have been discussed in the devel list.

[jazzynico (jazzynico)]

@David - Is that change absolutely necessary for your EMF/WMF work? As far as I can tell, it seems unrelated and should have been part of a different patch.

[David Mathog (mathog)]

To support EMF input of files with underline and strike-through Inkscape had to be able to display SVG text decoration. Look at bug #988601 post 171 and the examples shown there: text_decorations.svg and text_decorations.png. If the slight expansion does not take place then when one drags around decorated text the parts outside of the usual (undecorated) text region are not drawn correctly. Generally they leave a trail of line bits on the screen as they move, an effect which is much worse when the text is rotated. As you would expect, the parts most prone to this problem are overline (always), underline (some fonts), and the left or right ends of the strike-through.

There may be a way to define the selection area separately from the (re)draw area, but I'm not sure that entirely makes sense, because I think an end user would expect that clicking on a text decoration would also select.

Alternatively, the expanded area could be turned off if a test is first made to show that there are no text decorations. I didn't find the slightly expanded selection area to be a problem in my work, so never thought to implement that.

There is still no GUI to enter text decoration in Inkscape. I only added the bits and pieces needed to display it if it was already in the SVG, since EMF import works by writing an SVG file which Inkscape reads.

[Alvin Penner (apenner)]

There are two separate problems here:

1. the size of the vertical border is excessive. I have emulated what the ogv video showed, by using an 'a' and expanding it. I then measured the size of the letter (using the bbox) and the height of the (clickable) border. The height of the letter was 209 pixels, the full height of the clickable border was 489 pixels. The clickable border was 2.34 times larger than the letter. I can understand perhaps a padding of 10% or 20%, but an additional 134% is too much.

2. this is a very general problem, or issue, which has nothing to do with emf import, so it should be discussed outside the context of emf support.

[David Mathog (mathog)]

I just went back and tested the text decoration code, and it is broken. Most likely a victim of the scaling that was added a while back. That could easily explain why the clickable border is off so far. I will look into it.

[David Mathog (mathog)]

Here is a first step - restore the drawing of text decorations. Somebody modified my code and made the assumption that a transform operation was reversed on a certain scope change, but it wasn't, resulting in a double application of that transform and no text decorations drawn.

The next file I will post is the worst case scenario for how far the text decorations extend around text. As far as I can tell with the patch applied, the "+A" icon changes as it should at very close to the outermost edge of the text decorations. That is needed so that they draw properly. Those positions are, as you note, much further out than they were for undecorated text.

[David Mathog (mathog)]

The attached file is the worst case for the outer limits of text decorations - everything on using "wavy". Test the "+A" icon change as the cursor moves near it. On my machine it changes at very close to the outermost edge, for all 4 edges.

Now I will see if the bounding box can be tightened a little bit.

[David Mathog (mathog)]

The bounding box is determined in drawing-text.cpp at this line (87 in my version):

    Geom::Rect bigbox(Geom::Point(0.0, _asc*scale*1.1),Geom::Point(_width*scale, -_dsc*scale*1.1));

Changing both 1.1 to 1.0 causes problems on the worst case, some changes, like zooming in, result in half of the upper wave not being redrawn. So I tried changing it to 1.1 (for the ascent) and 1.0 (for the descent). That's when it got really, really weird. When set like that moving the cursor slowly in perpendicular to the 45 degree text the icon was first "+A", then it changed to the "bar I", then it changed BACK to the "+A", then it changed back finally, and the rest of the way through, to the "bar I". Bizarre. Still working on that...

Who added the section in this file that says:

  prepareFill / prepareStroke need to be called with _ctm in effect.

?? If I remove those changes, reverting back to 988601 (plus the format changes) the text decoration works again and I have not been able to find any defect in the text rendering. Both fills and stroke rotate and scale with the text in my tests. What feature is responsible for this "need"?

[David Mathog (mathog)]

Please try this new patch. It addresses:

1. Broken draws of text decorations. (By rearranging things so that changes go away when they go out of scope.)
2. Text selection icon not changing in the right place.
3. Having text fill scale and rotate with the glyphs.

The original code had a "delta" padding value for picking, outward by 7 (with no units). This patch just comments out that step, leaving a FIXME there to highlight the issue. If that step remains the selection is displaced outward from the width X ascender->descender rectangular bounding box, which doesn't seem to serve any purpose.

Now if I enter some text and then select all of it with the text tool, the text tool icon changes shape whenever it crosses the edge of the dark rectangle around the text. At least it does so if the text is oriented exactly horizontally or vertically, because there is apparently a very old bug (all the versions I tested) for text selection on rotated text. That is, the selection boundary appears to pixelate at a huge scale, resulting in a large staircase boundary. To see it:

1. Enter some text.
2. Rotate it 45 degrees.
3. Select the text tool
4. Select all the rotated text.
5. Put the text tool in the middle of the text and move it horizontally (slowly) to the right until it changes. The cursor is now on the staircase.
6. Move the cursor using only vertical and horizontal movements and one can trace out the staircase by noting thwere the cursor shape changes.

The staircase boundary's closest points are on the dark rectangle around the selected text. The edges of the staircase are always strictly vertical and horizontal, no matter what the angle of the text.

[Johan Engelen (johanengelen)]

Hi David,
 thanks for working on this. Please remember to declare variables whenever you can initialize them, and not sooner. There was also a duplicate variable naming in that function, so I had to rename it. I've made some other improvements to the code, see rev 12889.

[David Mathog (mathog)]

The patch you just added will not compile on my system.

The problem is in wmf-inout.cpp

extension/internal/wmf-inout.cpp: In static member function ‘static
double
Inkscape::Extension::Internal::Wmf::pix_to_x_point(Inkscape::Extension::Internal::PWMF_CALLBACK_DATA, double, double)’:
extension/internal/wmf-inout.cpp:871:12: error: ‘py’ was not declared in
this scope
extension/internal/wmf-inout.cpp: In static member function ‘static
double
Inkscape::Extension::Internal::Wmf::pix_to_y_point(Inkscape::Extension::Internal::PWMF_CALLBACK_DATA, double, double)’:
extension/internal/wmf-inout.cpp:880:12: error: ‘px’ was not declared in
this scope

That's because those functions used

    (void) py;

to quiet compiler warnings about the unused variable. You changed the
code to use

   double /*py*/

in the method declaration, but left in the void. They can't both be
there.

Regards,

David Mathog
<email address hidden>
Manager, Sequence Analysis Facility, Biology Division, Caltech

[su_v (suv-lp)]

<off-topic>
David Mathog (mathog) wrote:
> The patch you just added will not compile on my system.

AFAICT your local repo differs from upstream trunk (likely has local patches applied which you need to adjust to upstream changes): There is no '(void) py;' in current trunk:
<http://bazaar.launchpad.net/~inkscape.dev/inkscape/trunk/view/head:/src/extension/internal/wmf-inout.cpp#L1028>

Likely your build error comes from having the the patch for bug #1248753 (comment #10) applied which adds that line:
<https://launchpadlibrarian.net/156672753/changes_2013_11_15a.patch>
</off-topic>

[David Mathog (mathog)]

Sorry about 13, it wasn't supposed to go to this bug.

[Bryce Harrington (bryce)]

Sounds from comment #12 like rev 12889 implemented the changes proposed in mathong's patch landed in mainline. Subsequent comments are confusing.

Could someone summarize what remaining work is needed to get this bug report sorted?

[David Mathog (mathog)]

As far as I can tell my patch was not applied, or at least the parts that restored text-decoration rendering were not.

Note: bug 1269206 contains modifications to the Text and Font dialog (the one that opens with shift control T) which implement primitive editing of text decorations. That patch includes the changes in changes_2014_01_07a.patch. Otherwise there would be no way to see the text decorations to test the dialog.

[su_v (suv-lp)]

Still present (as originally reported) in r13167:
Attached sample file approximately visualizes the hit region for lowercase 'a' and uppercase 'A'. The blue area is the visual bounding box of each text object. 'Hit region' refers to the area where a left button mouse click selects the text object (instead of the rectangle stacked below the text object):
- blue: hit region in rev >= 12487
- red: hit region in rev >= 12488

Settings:
- Font: DejaVu Sans
- Grab sensitivity: 0 (requires restart).

This unexpected and visually not explicable extended hit area is disrupting workflows even in modestly complex drawings.

Even when support for text decorations lands in inkscape trunk, the visual bounding box and the hit area ought to be in sync, and really denote the extent of the current visual content of the object (it seems odd to me to "reserve" space just in the hit region for what might be added at some point (or not at all) by a text formatting property).

[Alvin Penner (apenner)]

+1

the clickable area and the visual bbox need to agree with each other, no matter what.

[David Mathog (mathog)]

Try this patch. Hopefully it will apply cleanly - my copy of drawing-text.cpp has the last patch from bug #1269206 applied but trunk doesn't.

The original pick box calculation used a method that dropped all space glyphs. As a consequence a line of text consisting of only spaces could not be selected with the text tool, and a line with trailing and leading spaces could only be selected at the outermost non-whitespace characters. To work around that the pick box was using font information including ascent and descent. Unfortunately for some fonts those values only poorly matched the actual glyphs, causing the problem seen in the present bug report.

The attached patch splits the difference. If there is a glyph, then it is used. If there isn't a glyph, like for a space character, the font information is used. In my hands with this patch the selection seems to be right on the outer bounding rectangle for text glyphs.

The other bounding box, the one which determines redraws, is still larger, so that text decorations will show.

I see one last issue, which I noted before in 988601 but could not find. When the text selection tool is active and the cursor goes into the right pickbox area a blue rectangle is shown. It corresponds to the pickbox EXCEPT that if there are trailing spaces the final one is dropped. Any idea where in Inkscape the code is that draws that blue rectangle?
 Example file will be in next post.

[David Mathog (mathog)]

This is an example of the trailing space issue. Open it (after the preceding patch is applied) and click somewhere in the text. Use the arrow keys to move to the right. You will see that the blue "selected" rectangle is one character short of the end of the string.

[Krzysztof Kosinski (tweenk)]

The visual bounding box should NOT include trailing / leading whitespace characters. They are not visually part of the object (it is called "visual bounding box" for a reason).

[David Mathog (mathog)]

I disagree. Not including white space in text makes text that is entirely white space unselectable, as in the attached example (at least with 0.48.4). It is easy to make these. In the example there were originally two pieces like the one that is still visible, to make one invisible the "a" and "b" were deleted and then I clicked elsewhere on the screen, which "lost" the piece I had been editing. My experimental text decoration additions to the text and font dialog (bug #1269206), did this all the time, by toggling the text decorations off on an otherwise blank line. Leading and trailing text are also not necessarily "invisible" when exported to other file types, due to background fill, for instance, so it is important to know their extent in the original SVG.

[ScislaC (scislac)]

A couple things here. While I agree that selecting these "invisible" objects should be easily doable with the Selector tool it doesn't merit trying to redefine Visual Bounding Box.

1) You can still select these items by using the Tab key.
2) You can also "select" them in the Text Tool by mouse. The visual cue here could be improved though for these objects, if it's a text object with only a space character, you have to pay attention to either the cursor changing or the text in the status bar to know when you're over it. I think it might be more helpful to have a better "helper path" (or box in this case) like we have for nodes, but for the text containers.

[Alvin Penner (apenner)]

I think this bug should be a blocker for the release of 0.48.5. It is inconceivable to me that we would release code with such a blatant bug in it.

[David Mathog (mathog)]

I still don't get what you folks think the problem is. Does the SVG spec say something about how the "Visual Bounding Box" is supposed to behave?

If there are two text elements like:

" "
and
 " a b "

before this patch a vertically moving 'arrow' or '+A' cursor would pass right through the terminal spaces without indicating that they were over a text element. (Conversely the +A passing over a center space would light up, so spaces were not treated equivalently for all positions in the string.) If those cursors moved horizontally across the two text objects in the first example it would never see the text object, in the second it would only see it once it was over "a b".

I know why I want to know when my cursor is over a text object, even if that location is just spaces. For instance, I would not want to start a second text object that overlapped the first by accident. Or I might want to remove leading and trailing spaces before sending the SVG out to some other graphic format.

What I don't get is why one would ever NOT want to know that the cursor was over a text object. What is the advantage in that? Whitespace is not visible (normally, it is if decorated), but it is still an integral part of the text.

I do see your point for graphical objects formed by clipping, where the outward extent of the original donut shaped clipping object is irrelevant - only the remaining clipped object viewed through the hole in that donut is relevant.

[ScislaC (scislac)]

Alvin: Is this an issue in 0.48.x? I did a pull and rebuilt and it didn't do that here.

[David Mathog (mathog)]

This is a prebult windows 32 bit r13276 with all the applicable patches applied (and some that are unrelated but shouldn't matter for this issue).

  http://saf.bio.caltech.edu/pub/software/windows/inkscape_r13276_trunk.zip

[Alvin Penner (apenner)]

@ScislaC - not reproduced on Inkscape 0.48.4. According to comment 2, this originated in rev 12488, merge of the EMF/WMF branch.

In order to see the full effect of the problem, it is necessary to resize the text, as shown in comment 1 video, and in comment 6.

[Alvin Penner (apenner)]

sorry, I think I misunderstood the question, I don't actually know if this is in 0.48.x or not.

[David Mathog (mathog)]

Alvin, is your system still doing this after the patch at post 20 is installed?

I don't see any indication that that patch was ever committed. If somebody could be kind enough to test it on an OS X system and it works there too, then go ahead and commit it. (Works for me on Linux and Windows.)

[su_v (suv-lp)]

Testing patch 'changes_2014_03_21a_pickonly.patch' from comment #20 with r13279 on OS X 10.7.5:
In the test case as described in comment #18, using the same sample SVG file attached there, I can confirm that the "hit region" is restored to the expected size of the visual bbox of each of the two text objects ('a', 'A').

I have not read in detail the discussion that followed (comments 20-26), about differences in the perception of what the visual bbox of text objects should include or not, nor done further testing based on that discussion (specifically wrt leading and trailing spaces) - based on this I won't commit the patch from comment #20 myself, and hope that one of the core developers can review it and if ok commit it as fix for the originally reported regression.

[su_v (suv-lp)]

@ScislaC - the regression only affects trunk (rev >= 12488), and thus could be considered release-critical for 0.91. The stable branch lp:inkscape/0.48.x is not affected (i.e. not an issue wrt releasing 0.48.5).

[ScislaC (scislac)]

Tested the most recent patch. It still selects too far outside the bbox where spaces are (as opposed to visible text). I do think the visual bounding box discussion should be moved to the mailing list though for wider discussion.

As of right now, I'm inclined to agree that leading/trailing spaces should be selectable because you're obviously intentionally adding them to modify the appearance... if we want to get nitpicky about Visual Bounding Box, there are filters that definitely extend much further than what is visually affected (e.g. Filters>Morphology>Black Hole).

[David Mathog (mathog)]

Re: 34, posted to developer list with subject "Should select area for text objects include leading and trailing spaces?"

[ScislaC (scislac)]

David: Just to make sure I wasn't too vague... with the patch, the space above/below spaces is still selectable even though it's fixed where there are visible characters.

[David Mathog (mathog)]

Understood.

For spaces there is a width but no glyph, so what to use for height? What is "above" for something that has no "top"? I went with the vertical extent as the ascent value from the font . The bottom of that selectable area should stay where it is on the baseline, but if people would prefer it we could use 1/2 or 3/4 ascent instead for the "top" of the selectable area of a space.

[ScislaC (scislac)]

Is there any way to determine the Cap Height? If not, I would be interested in testing 3/4 ascent. The bottom here doesn't seem like it's at the baseline, it's dipping below it into descender territory from what I'm witnessing. It's really unexpected if you aren't using any text with descenders... so if it did only go to the baseline that would be right imho.

Another side note, trailing spaces seem to extend outside of the bounding box & selcue (it seems like the last space isn't adjusting the "hit" area as much as a space anywhere else does).

[David Mathog (mathog)]

Re 38: You were right about the descender. I cannot make a diff right now, working on something else and lots of other changes are in there, but I tried this change in src/display/drawing-text.cpp (line 99 in my copy):

Geom::Rect pbigbox(Geom::Point(0.0, _asc*scale_bigbox*1.0),Geom::Point(_width*scale_bigbox, -_dsc*scale_bigbox*1.0));

   to

Geom::Rect pbigbox(Geom::Point(0.0, _asc*scale_bigbox*0.75),Geom::Point(_width*scale_bigbox, 0));

and the bottom is now at the baseline. With 0.75 the space is selected a little higher up than a lower case "a".

You are correct that the trailing spaces, if present extend one space past the blue box when text is selected. That does not seem to be in this section of code. I looked for it once but didn't find it. Almost certainly an instance where a loop condition is a "<" when it should be "<=".

[David Mathog (mathog)]

Try this patch, it replaces the preceding one.

[ScislaC (scislac)]

Here it is at 2/3... this feels much more comfortable to me. I tested it with about a dozen very different fonts and while it feels like it reaches a little high on some fonts, on other it would be too low... so it seems like a reasonable compromise (at least imho).

[David Mathog (mathog)]

It does not matter to me if it is 2/3 or 3/4. If you like 2/3 go ahead and commit it.

[David Mathog (mathog)]

Slight modification of preceding patch. This one extends the pick region to include text decorations. (This was done for all characters by default originally, which is what caused this bug in the first place.) Now it is only done for characters that actually have text decorations.

The method is still an approximation. The text decorations themselves are not drawn until much later, and their exact positions are not known where the pick box is defined, but this is pretty close for simple underline,overline. For the wavy and double formats, coming in the next SVG, the selection point is about in the middle of the decoration.

[Alvin Penner (apenner)]

just bumping this report.

I would vote for committing the patch from comment 41 above, since this is the last patch that has actually been tested.

[ScislaC (scislac)]

Didn't need to be bumped, I just wasn't sure if David was going to commit since he has since gained commit access.

David: I tried your last patch in comment 43 and it failed to compile...

display/drawing-text.cpp: In member function ‘virtual unsigned int Inkscape::DrawingGlyphs::_updateItem(const IntRect&, const Inkscape::UpdateContext&, unsigned int, unsigned int)’:
display/drawing-text.cpp:108:9: error: ‘_decorations’ was not declared in this scope
     if (_decorations.overline || _decorations.underline) {

[David Mathog (mathog)]

Tav's changes to style broke text decoration in general and this in particular. I'm working on getting a patch together to make it work again, and will include the patch for this in it. Actually, the pick box part is already in what I have done so far, but the part that sets the fill and stroke for text decoration conditionally on fill, stroke, and text_decoration svg fills is not yet working.

[Bryce Harrington (bryce)]

This bug has been selected as Bug of the Week for the week of May 12. If it is closed by May 18th it will count as double points for the Bug Hunt. (It will also get double points since it is a regression.)

[David Mathog (mathog)]

It should be fixed in rev 13372.

In this version pick ignores text decorations. If the cursor passes over an over- or underline nothing happens, it is only when it is over the letter itself that the text may be selected. For now this makes sense, since at present there are no operations that may be performed on just the text decoration requiring that it be pickable.

Leading and trailing spaces are pickable within the box of size "space width" X "2/3 font height".

Unrelated remaining bug which may be confused with this one - selected text with trailing spaces is surrounded by a blue line bounding box that is one space less in length than the actual text. If the cursor moves into such a piece of text from the right it looks like the text was selected one character too far to the right, but what is actually happening is that the "selected" box is drawn incorrectly.

[su_v (suv-lp)]

Fix wrt originally report issue confirmed with r13372 on OS X 10.7.5.

Remaining issues wrt leading/trailing spaces and the size of the sel cue (dashed, or blue) should be tracked in separate reports.