Bug 106132 - FILEOPEN, DOCX: Frame (text box) around image cuts into image and caption
Summary: FILEOPEN, DOCX: Frame (text box) around image cuts into image and caption
Status: RESOLVED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Writer (show other bugs)
Version:
(earliest affected)
5.3.0.3 release
Hardware: All All
: high major
Assignee: Miklos Vajna
URL:
Whiteboard: interoperability target:6.0.0 target:...
Keywords: bibisected, bisected, filter:docx, regression
: 103990 (view as bug list)
Depends on:
Blocks: DOCX-Textbox
  Show dependency treegraph
 
Reported: 2017-02-22 00:54 UTC by Aron Budea
Modified: 2020-03-18 13:36 UTC (History)
4 users (show)

See Also:
Crash report or crash signature:


Attachments
Sample, save as DOCX and reopen (10.86 KB, application/vnd.oasis.opendocument.text)
2017-02-22 00:54 UTC, Aron Budea
Details
Save as .docx and reopen to reproduce the bug (31.52 KB, application/vnd.oasis.opendocument.text)
2017-04-27 11:52 UTC, Cesar Martinez Izquierdo
Details
Sample of the resulting .docx file (13.27 KB, application/vnd.openxmlformats-officedocument.wordprocessingml.document)
2017-04-27 11:53 UTC, Cesar Martinez Izquierdo
Details
DOCX-independent reproducer (ODT) (10.70 KB, application/vnd.oasis.opendocument.text)
2017-06-23 20:32 UTC, Miklos Vajna
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Aron Budea 2017-02-22 00:54:47 UTC
Created attachment 131403 [details]
Sample, save as DOCX and reopen

1. Insert an image in an empty document.
2. Set a caption via right click on image, Insert Caption... (just add a caption and click OK)
3. Save file as DOCX.
4. Reopen in Writer.

An ODT with steps 1-2 is attached.

=> The frame around the image is incorrectly sized, and can't contain the image or caption anymore.
(there's no numbering, either, that will be reported separately)

Reproduced with 5.3.0.3 / Windows 7, looks fine in 5.2.0.4. => regression
Interestingly, since 4.4.0.3 the frame also turns into a text box, which, if created manually, can't hold images.

Since it's a basic function, setting importance to high/major.
Comment 1 Aron Budea 2017-02-22 00:56:02 UTC Comment hidden (bibisection)
Comment 2 Aron Budea 2017-02-22 00:57:43 UTC
The commit is also causing bug 103990, but it's hard to determine whether the two issues are the exact same, and also the repro steps/document are very simple here.
Adding Cc: to Miklos Vajna, please take a look.

https://cgit.freedesktop.org/libreoffice/core/commit/?id=c761df1e42fd11acc5fc05b0baacd803c3788ca6
author		Miklos Vajna <vmiklos@collabora.co.uk>	2016-10-25 07:05:47 (GMT)
committer	Miklos Vajna <vmiklos@collabora.co.uk>	2016-10-25 09:32:01 (GMT)

"tdf#84678 DOCX import: fix handling of textbox margins"
Comment 3 Xisco Faulí 2017-02-23 10:05:36 UTC
Confirmed in

Version: 5.4.0.0.alpha0+
Build ID: 880033edde516fc30225005245253293a6a58ba4
CPU Threads: 4; OS Version: Linux 4.8; UI Render: default; VCL: gtk2; 
Locale: ca-ES (ca_ES.UTF-8); Calc: group

I guess both bugs are related. Anyway, let's keep both open and see if fixing one fixes the other...
Comment 4 Cesar Martinez Izquierdo 2017-04-27 11:51:32 UTC Comment hidden (off-topic)
Comment 5 Cesar Martinez Izquierdo 2017-04-27 11:52:37 UTC Comment hidden (off-topic)
Comment 6 Cesar Martinez Izquierdo 2017-04-27 11:53:25 UTC Comment hidden (off-topic)
Comment 7 Miklos Vajna 2017-06-23 20:32:34 UTC
Created attachment 134236 [details]
DOCX-independent reproducer (ODT)

I think the root cause here is somewhere in Writer, not in the docx filter. Reverting the bisected commit doesn't help here. Here is what I tried: if I save the old import result in ODT, then that ODT is loaded fine in the parent of the bisected commit (i.e. something between 5.2 and 5.3), but loading the same ODT on master is broken. I *think* this is the root cause.

Bibisecting this breakage gives the 8f2dd1df1d6cc94ebbc1149de72bc6d6dffa6533..a6ce5d391476e4b6a2cb2d92ff45548c1d75684b range, from which 5d9d0f3c979732ade57b9c4c4960dd030ffdc9f9 looks related.

It's not clear to me what is the problem, though. There was a follow-up fix of that commit as 1427817a944f3cf1020b2f06a2ca934847b56ba8. Reverting both of these commits locally build, but the bug does not go away.
Comment 8 Miklos Vajna 2017-06-23 20:34:29 UTC
Justin, any idea what is going on? I guess the root cause here is somewhere around this nice new sw feature that you can have margins without borders, but I'm not sure. Thanks.
Comment 9 Miklos Vajna 2017-06-23 21:31:05 UTC
(Yes, in the meantime I bisected with bibisect-linux-64-53.git and git bisect points out exactly the commit I suspected. So no more guessing the commit from the range.)
Comment 10 Justin L 2017-06-23 23:06:21 UTC
(In reply to Miklos Vajna from comment #9)
> (Yes, in the meantime I bisected with bibisect-linux-64-53.git and git
> bisect points out exactly the commit I suspected.
"there is a function for that: CalcLineSpace(xx, bEvenIfNoLine)" was a VERY unfortunately commit because it prematurely causes the regressions that tdf#41542 (globally allow borderless padding) would have made the following day.
Comment 11 Justin L 2017-06-24 01:37:22 UTC
My guess is that some default spacing values for frames/textboxes are not being reset to zero during the conversion. Before, unnecessary values would have been ignored, but as soon as AllowSpacingWithoutBorders exists (which set frmtool.cxx::m_bBorderDist = true), then the entire frame shifted down. Thus, both Miklos and my commits are basically unmasking the same error.

Spacing numbers are 142 for right/left and 71 for top/bottom. If that matches defaults, that would validate my suspicions.
Comment 12 Justin L 2017-07-03 23:54:01 UTC
(In reply to Justin L from comment #11)
> My guess is that some default spacing values for frames/textboxes are not
> being reset to zero during the conversion.
void Shape::setDefaults(bool bHeight)
{
    maDefaultShapeProperties.setProperty(PROP_TextLeftDistance, static_cast< sal_Int32 >( 250 ));
    maDefaultShapeProperties.setProperty(PROP_TextUpperDistance, static_cast< sal_Int32 >( 125 ));
    maDefaultShapeProperties.setProperty(PROP_TextRightDistance, static_cast< sal_Int32 >( 250 ));
    maDefaultShapeProperties.setProperty(PROP_TextLowerDistance, static_cast< sal_Int32 >( 125 ));
}

Potential fix for the .docx documents (except that it isn't focused on this particular bug and likely would cause lots of regressions) is:
+++ b/oox/source/drawingml/shape.cxx
@@ -899,6 +899,12 @@ Reference< XShape > const & Shape::createAndInsert(
else if (mbTextBox)
{
  aShapeProps.setProperty(PROP_TextBox, true);
+//clear default border spacing values
+aShapeProps.setProperty(PROP_TextLeftDistance,  static_cast< sal_Int32 >( 0 ));
+aShapeProps.setProperty(PROP_TextUpperDistance, static_cast< sal_Int32 >( 0 ));
+aShapeProps.setProperty(PROP_TextRightDistance, static_cast< sal_Int32 >( 0 ));
+aShapeProps.setProperty(PROP_TextLowerDistance, static_cast< sal_Int32 >( 0 ));
}
Comment 13 Justin L 2017-07-04 01:26:07 UTC Comment hidden (off-topic)
Comment 14 Justin L 2017-07-04 02:36:05 UTC
*** Bug 103990 has been marked as a duplicate of this bug. ***
Comment 15 Justin L 2017-07-04 02:39:06 UTC
Miklos: Can you take over on this one? My hack can be more closely targeted with a if( getWps() ) clause, but you have the big picture scope needed for this.
Comment 16 Miklos Vajna 2017-07-05 08:52:02 UTC
Justin: I think the root cause is in sw/, not in the import filter. Some context if that helps:

Shapes from DOCX are imported as "shape with textbox", which means that we create a draw shapes (which can have rounded corners, etc) and then there is "textbox" attached to it, which is a Writer textframe (so can contain tables, etc). The UI and UNO API hides direct access to this Writer textframe.

Now OOXML shapes can have zero margin for their text (even if they don't have a border), and in the textbox use-case the Writer textframes never have a border, that's how your sw feature was super-useful for me, now I can have zero margin and no borders at the same time, needed by the "zero margin in OOXML shape with text" case.

The getWps() info is available only in oox. See the above ODT document I attached, I think this doesn't have much to do with OOXML.

> Miklos: Can you take over on this one?

Ah, so regarding the above ODT file, you think there is no bug in general, but only when it comes to textboxes?

I rather hoped you would take this over, but sure, if zero-margin without borders work in general and this is specific to textboxes, then I can try to take a look. :-)
Comment 17 Justin L 2017-07-05 13:14:11 UTC
(In reply to Miklos Vajna from comment #16)
> Ah, so regarding the above ODT file, you think there is no bug in general,
> but only when it comes to textboxes?
Correct. I think the .ODT is just "honoring" the hidden textbox border padding settings that were created as part of the .docx import (since the file originated as .docx).

So, I'm pretty sure it is just the "conversion to shape with textframe containing non-zero padding defaults" that is the problem here. However, the shape code is so generic that I have no idea what all I would be affecting if I made changes there.

from-docx-old.odt contains
...style:style style:name="gr1" ... fo:padding-top="0.125cm" fo:padding-bottom="0.125cm" fo:padding-left="0.25cm" fo:padding-right="0.25cm"...

Any re-saved .docx -> odt files since LO4.4 (2014-06-18 09:57:31) d379d18666aa42031359ca8eb34b0021960347ae will probably be "unfixable" since they will have the default border padding hard-coded inside.
Comment 18 Miklos Vajna 2017-07-07 13:11:04 UTC
OK, I'm taking this.
Comment 19 Miklos Vajna 2017-07-11 12:09:43 UTC
Justing, thanks a lot for suggesting this is a filter / textbox bug, not an sw layout problem, you were right. :-)
Comment 20 Commit Notification 2017-07-11 12:10:52 UTC
Miklos Vajna committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=6f9f9491bdef676f969898bd653db9905d14f5e8

tdf#106132 DOCX import: fix handling of nested textbox margins

It will be available in 6.0.0.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 21 Commit Notification 2017-07-18 13:27:17 UTC
Miklos Vajna committed a patch related to this issue.
It has been pushed to "libreoffice-5-4":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=2cd91d79b90316f6a21db64bd944cb828b345a78&h=libreoffice-5-4

tdf#106132 DOCX import: fix handling of nested textbox margins

It will be available in 5.4.1.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.