bug#22250: 25.0.50; Eww fails to break RTL paragraph

bug#22250: 25.0.50; Eww fails to break RTL paragraph

[Benjamin Riefenstahl]

[-- Attachment #1: Type: text/plain, Size: 283 bytes --]

I load the attached HTML file in a git Emacs (branch emacs-25, revision
f9d87dd) with eww-open-file.  Emacs shows two lines with the first one
being very long, instead of wrapping the paragraph as expected.  The
command I execute:

  ./emacs -Q --eval '(eww-open-file "test.html")'


[-- Attachment #2: test.html --]
[-- Type: text/html, Size: 430 bytes --]

[-- Attachment #3: eww.png --]
[-- Type: image/png, Size: 20759 bytes --]

bug#22250: 25.0.50; Eww fails to break RTL paragraph

[Eli Zaretskii]

> From: Benjamin Riefenstahl <b.riefenstahl@turtle-trading.net>
> Date: Sun, 27 Dec 2015 20:11:52 +0100
> Cc: Benjamin Riefenstahl <b.riefenstahl@turtle-trading.net>
> 
> I load the attached HTML file in a git Emacs (branch emacs-25, revision
> f9d87dd) with eww-open-file.  Emacs shows two lines with the first one
> being very long, instead of wrapping the paragraph as expected.  The
> command I execute:
> 
>   ./emacs -Q --eval '(eww-open-file "test.html")'

Out of curiosity: why are you declaring a paragraph consisting
entirely of L2R text as R2L?  is there some real-life use case behind
this?

(If you replace "abc" with "אבג", the display will be as you expect.)

bug#22250: 25.0.50; Eww fails to break RTL paragraph

[Lars Ingebrigtsen]

Benjamin Riefenstahl <b.riefenstahl@turtle-trading.net> writes:

> I load the attached HTML file in a git Emacs (branch emacs-25, revision
> f9d87dd) with eww-open-file.  Emacs shows two lines with the first one
> being very long, instead of wrapping the paragraph as expected.  The
> command I execute:

I can confirm that this doesn't work very well.  For me it seems to
display just a single line?

Do you know whether this used to work, and stopped working after the
recent directional fixes, by any chance?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#22250: 25.0.50; Eww fails to break RTL paragraph

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Date: Sun, 27 Dec 2015 20:30:40 +0100
> Cc: 22250@debbugs.gnu.org
> 
> Do you know whether this used to work, and stopped working after the
> recent directional fixes, by any chance?

Where's the code that breaks such long paragraphs into lines?

bug#22250: 25.0.50; Eww fails to break RTL paragraph

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Date: Sun, 27 Dec 2015 20:30:40 +0100
> Cc: 22250@debbugs.gnu.org
> 
> Do you know whether this used to work, and stopped working after the
> recent directional fixes, by any chance?

Before those directional fixes the value of bidi-paragraph-direction
was 'left-to-right', i.e. it ignored the dir=rtl meta-data.  So
obviously, the problem didn't exist back then, but it didn't start
_because_ of that change.  It's just that the change you made exposed
a bug in the line-breaking algorithm -- it somehow assumes
unidirectional text somewhere.  If you manually set
bidi-paragraph-direction to 'left-to-right', after visiting that file,
the problem disappears, leaving on screen just the incorrect display.

If you can point me to the code which does that, I might find the
problem and suggest the solution.

Thanks.

bug#22250: 25.0.50; Eww fails to break RTL paragraph

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> If you can point me to the code which does that, I might find the
> problem and suggest the solution.

It's `shr-fill-line'.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#22250: 25.0.50; Eww fails to break RTL paragraph

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: b.riefenstahl@turtle-trading.net,  22250@debbugs.gnu.org
> Date: Sun, 27 Dec 2015 20:49:25 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > If you can point me to the code which does that, I might find the
> > problem and suggest the solution.
> 
> It's `shr-fill-line'.

Thanks.

There's nothing wrong with eww or shr.  This is the (strange-looking,
but expected) result of displaying a long line of LTR text in a RTL
paragraph with truncate-lines set to t.  Here, try this:

  emacs -Q
  C-x b foo RET
  M-x set-variable RET bidi-paragraph-direction RET right-to-left RET
  M-x set-variable RET truncate-lines RET t RET

Now press and hold 'a', let it fill the window line and hscroll, then
type RET, and repeat for another line.  Finally, type M-<.  You will
see exactly the same display as shown in the original report.  It
should now be clear this has nothing to do with either eww or breaking
paragraphs in HTML pages.

So now I have to ask Benjamin why does he think there's a bug here.
Because I don't see any bug.  I do agree that the display looks
strange, but that's what you get for displaying long lines of LTR text
in a RTL paragraph with truncate-lines set to t.  RTL paragraphs are
for predominantly RTL text, exactly like LTR paragraphs are for
predominantly LTR text.

bug#22250: 25.0.50; Eww fails to break RTL paragraph

[Eli Zaretskii]

> Date: Sun, 27 Dec 2015 22:22:34 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: 22250@debbugs.gnu.org, b.riefenstahl@turtle-trading.net
> 
> So now I have to ask Benjamin why does he think there's a bug here.
> Because I don't see any bug.  I do agree that the display looks
> strange, but that's what you get for displaying long lines of LTR text
> in a RTL paragraph with truncate-lines set to t.  RTL paragraphs are
> for predominantly RTL text, exactly like LTR paragraphs are for
> predominantly LTR text.

Ignore me, I didn't think clearly.  I see the problem.

bug#22250: 25.0.50; Eww fails to break RTL paragraph

[Eli Zaretskii]

> Date: Sun, 27 Dec 2015 22:28:20 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: 22250@debbugs.gnu.org, b.riefenstahl@turtle-trading.net
> 
> > Date: Sun, 27 Dec 2015 22:22:34 +0200
> > From: Eli Zaretskii <eliz@gnu.org>
> > Cc: 22250@debbugs.gnu.org, b.riefenstahl@turtle-trading.net
> > 
> > So now I have to ask Benjamin why does he think there's a bug here.
> > Because I don't see any bug.  I do agree that the display looks
> > strange, but that's what you get for displaying long lines of LTR text
> > in a RTL paragraph with truncate-lines set to t.  RTL paragraphs are
> > for predominantly RTL text, exactly like LTR paragraphs are for
> > predominantly LTR text.
> 
> Ignore me, I didn't think clearly.  I see the problem.

The problem is that when you have a LTR line in RTL paragraph, you
start filling line not at window edge, and the window is already
hscrolled.

I need to think how to fix this cleanly.

Meanwhile, can you tell what is that 'shr-indentation' property used
for?  It looks related to filling lines.

bug#22250: 25.0.50; Eww fails to break RTL paragraph

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> Meanwhile, can you tell what is that 'shr-indentation' property used
> for?  It looks related to filling lines.

shr-indentation says how far lines should be indented.  It's mainly used
for <ul>/<li> elements:

* This starts here and
  folds down to here, so
  shr-indentation is three
  characters or the equivalent
  number of pixels if using
  variable width fonts.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#22250: 25.0.50; Eww fails to break RTL paragraph

[Benjamin Riefenstahl]

[-- Attachment #1: Type: text/plain, Size: 1609 bytes --]

Eli Zaretskii writes:
> Out of curiosity: why are you declaring a paragraph consisting
> entirely of L2R text as R2L?  is there some real-life use case behind
> this?

As you probably guessed this is a radical simplification of the actual
problem that I have.

> (If you replace "abc" with "אבג", the display will be as you expect.)

Interesting.  It seems that I have simplyfied it a bit too much.
Although the version that I created still indicates a problem, of
course.

I experimented a bit and I can't quite exactly say what is needed to
cause the problem.  The same file/URL has it and than again not when I
try again.  It seems that "g" usually fixes it, while "G RET" usually
reproduces it, if it has happend with that particular file/URL before.
It also seems that the length of the URL has an impact, probably because
it is shown at the top of the frame and during loading of the URL (as in
"Loading http[...]").  It seems to require that the URL is longer than
the frame width.

I have no sure recipe using a file, but I have something based on a
simple 10-line HTTP server look-alike, see attachments.  Put the files
in some directory, make the script executable, execute it, and call EWW
with the URL
http://127.0.0.1:1234/abc/abc/abc/abc/abc/abc/abc/abc/abc/abc/abc/abc/.
If it looks good on the first call, try "G RET", than it reliably
exhibits the problem for me.  I even get it with "emacs -Q -nw".  Note
that the script depends on netcat and different versions of netcat use
different options to run a server, so the script may not work as-is for
you.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: make-html.sh --]
[-- Type: text/x-sh, Size: 396 bytes --]

#!/bin/bash

function execute () {
    sleep .5

    echo -e "HTTP/1.0 200 OK\r"
    echo -e "Content-Type: text/html; charset=utf-8\r"
    echo -e "\r"

    cat test.html
}

function loop () {
    while sleep 1; do
        echo Start
        netcat -l -s localhost -p 1234 -c "$0 execute"
        echo Handled one request
    done
}

if [ 0 = $# ]; then
    loop
else
    "$@"
fi

#### eof ####

[-- Attachment #3: test.html --]
[-- Type: text/html, Size: 683 bytes --]

bug#22250: 25.0.50; Eww fails to break RTL paragraph

[Eli Zaretskii]

> From: Benjamin Riefenstahl <b.riefenstahl@turtle-trading.net>
> Cc: 22250@debbugs.gnu.org, Lars Ingebrigtsen <larsi@gnus.org>
> Date: Mon, 28 Dec 2015 00:09:51 +0100
> 
> I experimented a bit and I can't quite exactly say what is needed to
> cause the problem.  The same file/URL has it and than again not when I
> try again.  It seems that "g" usually fixes it, while "G RET" usually
> reproduces it, if it has happend with that particular file/URL before.
> It also seems that the length of the URL has an impact, probably because
> it is shown at the top of the frame and during loading of the URL (as in
> "Loading http[...]").  It seems to require that the URL is longer than
> the frame width.
> 
> I have no sure recipe using a file, but I have something based on a
> simple 10-line HTTP server look-alike, see attachments.  Put the files
> in some directory, make the script executable, execute it, and call EWW
> with the URL
> http://127.0.0.1:1234/abc/abc/abc/abc/abc/abc/abc/abc/abc/abc/abc/abc/.
> If it looks good on the first call, try "G RET", than it reliably
> exhibits the problem for me.  I even get it with "emacs -Q -nw".  Note
> that the script depends on netcat and different versions of netcat use
> different options to run a server, so the script may not work as-is for
> you.

Does the patch below fix the problem?

Lars, we don't seem to have a test suite for eww or shr, so if you
have some layout tests, including with the indentation you explained
above, could you try with this patch and see if anything breaks?

Thanks.

diff --git a/lisp/net/shr.el b/lisp/net/shr.el
index c28e0b8..f7da3e9 100644
--- a/lisp/net/shr.el
+++ b/lisp/net/shr.el
@@ -244,7 +244,8 @@ shr-insert-document
                                      (if (and (null shr-width)
                                               (not (shr--have-one-fringe-p)))
                                          (* (frame-char-width) 2)
-                                       0))))))
+                                       0)))))
+        bidi-display-reordering)
     (shr-descend dom)
     (shr-fill-lines start (point))
     (shr-remove-trailing-whitespace start (point))

bug#22250: 25.0.50; Eww fails to break RTL paragraph

[Benjamin Riefenstahl]

Eli Zaretskii writes:
> Does the patch below fix the problem?

Sorry, no, no change here with that. 


I looked at the code though and I noticed the call stack

  shr-fill-line
   -> shr-vertical-motion
      -> vertical-motion

which is used to find the line break point.  It seems the idea here is
that Emacs' normal line wrap is used, we just go one line down, and
vertical-motion gets us to the place where Emacs has wrapped.  But
Emacs' line wrap is only active with truncate-lines=nil, and
truncate-lines is set to t in eww-mode.  When I set truncate-lines=nil
in shr-insert-document the bug does actually go away.  I do not know why
truncate-lines was set to t in the first place, though, and where that
may have other consequences.


I'm wondering why this bug depends on RTL layout.  I tried my test case
without dir=rtl and with "abc" as text, but that works fine, so RTL
really is a factor.

But that's not right, is it?  Conceptually, the linebreak opportunities
should be the same regardless of the paragraph direction, RTL or LTR.
For mixed text the visual order of the runs for should be different for
RTL paragraphs, but even than the length of the lines and where they are
broken should not change.  Line breaks should be found according to
logical ordering.  Let's say I have:

   aaa bbb אאא בבב

If I have to break it before the last word, I want to get:

   aaa bbb אאא
   בבב

And indeed, fill-paragraph does the right thing with this example.

If I narrow my Emacs window here, it breaks like this, though:

   aaa bbb בבב
   אאא

So there seems to be an even bigger issue in Emacs generally?  At least
Emacs default wrapping behaviour seems not directly usable as a basis
for filling paragraphs correctly.

bug#22250: 25.0.50; Eww fails to break RTL paragraph

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> Lars, we don't seem to have a test suite for eww or shr, so if you
> have some layout tests, including with the indentation you explained
> above, could you try with this patch and see if anything breaks?

No, I don't have any layout tests...

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#22250: 25.0.50; Eww fails to break RTL paragraph

[Eli Zaretskii]

> From: Benjamin Riefenstahl <b.riefenstahl@turtle-trading.net>
> Cc: 22250@debbugs.gnu.org,  larsi@gnus.org
> Date: Mon, 28 Dec 2015 17:40:43 +0100
> 
> Eli Zaretskii writes:
> > Does the patch below fix the problem?
> 
> Sorry, no, no change here with that. 

??? It certainly solved the problem with your original HTML file
visited with eww-open-file.  Doesn't it solve that for you?  Are you
sure you tested correctly?  And what exactly did you test?

> I looked at the code though and I noticed the call stack
> 
>   shr-fill-line
>    -> shr-vertical-motion
>       -> vertical-motion
> 
> which is used to find the line break point.  It seems the idea here is
> that Emacs' normal line wrap is used, we just go one line down, and
> vertical-motion gets us to the place where Emacs has wrapped.

No, that's not how this works, AFAIK.  (But Lars can correct me if I'm
wrong.)  What it does is go to the column where it wants to wrap and
then checks if it's at EOL.  If not, it goes back and looks for a
place to insert a newline.  Rinse, repeat.

> I'm wondering why this bug depends on RTL layout.

I'm wondering why it happens at all, after the patch I sent.  When
bidi-display-reordering is set to nil, there is no RTL layout, any
text is laid out in strict logical left-to-right order.

> I tried my test case without dir=rtl and with "abc" as text, but
> that works fine, so RTL really is a factor.

It cannot be, not when bidi-display-reordering is bound to nil.
Something else is at work there.

> If I narrow my Emacs window here, it breaks like this, though:
> 
>    aaa bbb בבב
>    אאא
> 
> So there seems to be an even bigger issue in Emacs generally?

No, this is the bidi display engine working as designed.

> At least Emacs default wrapping behaviour seems not directly usable
> as a basis for filling paragraphs correctly.

shr doesn't use the wrapping behavior, it does its own layout
calculations.

bug#22250: 25.0.50; Eww fails to break RTL paragraph

[Eli Zaretskii]

> Date: Mon, 28 Dec 2015 19:12:00 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: 22250@debbugs.gnu.org, larsi@gnus.org
> 
> > I'm wondering why this bug depends on RTL layout.
> 
> I'm wondering why it happens at all, after the patch I sent.  When
> bidi-display-reordering is set to nil, there is no RTL layout, any
> text is laid out in strict logical left-to-right order.
> 
> > I tried my test case without dir=rtl and with "abc" as text, but
> > that works fine, so RTL really is a factor.
> 
> It cannot be, not when bidi-display-reordering is bound to nil.
> Something else is at work there.

The only way I can explain this to myself is by reasoning that
whatever code is related to the problem you see is somewhere else, not
in shr-fill-lines or its subroutines, because while shr-fill-lines
runs, my patch makes LTR and RTL text indistinguishable.  So it's some
other code that is responsible.

It's important to understand what exactly are you trying, because your
original test case in this bug report is fixed by the patch I sent.

Thanks.

bug#22250: 25.0.50; Eww fails to break RTL paragraph

[Benjamin Riefenstahl]

>> Eli Zaretskii writes:
>> > Does the patch below fix the problem?

>> From: Benjamin Riefenstahl <b.riefenstahl@turtle-trading.net>
>> Sorry, no, no change here with that. 

Eli Zaretskii writes:
> ??? It certainly solved the problem with your original HTML file
> visited with eww-open-file.  Doesn't it solve that for you?  Are you
> sure you tested correctly?  And what exactly did you test?

Ah, I'm sorry, I forgot about that one, my bad.  You are right, my first
test case is indeed fixed by your patch.

Just my actual problem is not, including the last script-based test case
I made.  (Which I was working on the whole evening yesterday :-()

>> I looked at the code though and I noticed the call stack
>> 
>>   shr-fill-line
>>    -> shr-vertical-motion
>>       -> vertical-motion
>> 
>> which is used to find the line break point.  It seems the idea here is
>> that Emacs' normal line wrap is used, we just go one line down, and
>> vertical-motion gets us to the place where Emacs has wrapped.
>
> No, that's not how this works, AFAIK.  (But Lars can correct me if I'm
> wrong.)  What it does is go to the column where it wants to wrap and
> then checks if it's at EOL.  If not, it goes back and looks for a
> place to insert a newline.  Rinse, repeat.

Fact is that vertical-motion is called in the course of determining the
break (I think as part of "go to the column where it wants to wrap") and
that my actual problems and the script test case seem to be solved by
setting truncate-lines to nil.

We have two different bugs here, because my first test case is *not*
fixed just by setting truncate-lines to nil.

>> I'm wondering why this bug depends on RTL layout.

Here I was talking about the script test case I sent, not the first test
case, which I had entirely forgotten about.

> I'm wondering why it happens at all, after the patch I sent.  When
> bidi-display-reordering is set to nil, there is no RTL layout, any
> text is laid out in strict logical left-to-right order.
>
>> I tried my test case without dir=rtl and with "abc" as text, but
>> that works fine, so RTL really is a factor.
>
> It cannot be, not when bidi-display-reordering is bound to nil.
> Something else is at work there.

Again, I am sorry for the confusion.

bug#22250: 25.0.50; Eww fails to break RTL paragraph

[Eli Zaretskii]

> From: Benjamin Riefenstahl <b.riefenstahl@turtle-trading.net>
> Cc: 22250@debbugs.gnu.org,  larsi@gnus.org
> Date: Mon, 28 Dec 2015 19:15:50 +0100
> 
> >> Eli Zaretskii writes:
> >> > Does the patch below fix the problem?
> 
> >> From: Benjamin Riefenstahl <b.riefenstahl@turtle-trading.net>
> >> Sorry, no, no change here with that. 
> 
> Eli Zaretskii writes:
> > ??? It certainly solved the problem with your original HTML file
> > visited with eww-open-file.  Doesn't it solve that for you?  Are you
> > sure you tested correctly?  And what exactly did you test?
> 
> Ah, I'm sorry, I forgot about that one, my bad.  You are right, my first
> test case is indeed fixed by your patch.

I'm relieved.  Thanks.

> Just my actual problem is not, including the last script-based test case
> I made.  (Which I was working on the whole evening yesterday :-()

Is there any way of recreating that problem without having to set up
netcat?  (Why is receiving a page from a server different from
visiting the same page as a disk file?  The way shr works, this should
produce exactly the same results, no?)

> >> I looked at the code though and I noticed the call stack
> >> 
> >>   shr-fill-line
> >>    -> shr-vertical-motion
> >>       -> vertical-motion
> >> 
> >> which is used to find the line break point.  It seems the idea here is
> >> that Emacs' normal line wrap is used, we just go one line down, and
> >> vertical-motion gets us to the place where Emacs has wrapped.
> >
> > No, that's not how this works, AFAIK.  (But Lars can correct me if I'm
> > wrong.)  What it does is go to the column where it wants to wrap and
> > then checks if it's at EOL.  If not, it goes back and looks for a
> > place to insert a newline.  Rinse, repeat.
> 
> Fact is that vertical-motion is called in the course of determining the
> break (I think as part of "go to the column where it wants to wrap")

Yes, but vertical-motion is only used here to go to a particular
column on the same line.  It never goes down or up.  How's that
related to wrapping lines?

> and that my actual problems and the script test case seem to be
> solved by setting truncate-lines to nil.

I understand, but that doesn't yet explain what exactly doesn't work
correctly.  I'm pretty sure that the code which is responsible is not
in shr-fill-line, because when that runs, the bidi reordering is
disabled by my patch.

> We have two different bugs here, because my first test case is *not*
> fixed just by setting truncate-lines to nil.

Yes.  The question is where is the other code.  Can you spot that?

Failing that, can you describe what exactly do you see, or maybe step
through the code in Edebug and tell which part(s) misbehave?

bug#22250: 25.0.50; Eww fails to break RTL paragraph

[Benjamin Riefenstahl]

[-- Attachment #1: Type: text/plain, Size: 248 bytes --]

Benjamin Riefenstahl writes:
> I have no sure recipe using a file,

Now I have one, see attachment.  This, too, is fixed here by setting
truncate-lines to nil in eww-mode.  The file can be loaded just through
eww-open-file to exhibit the problem.


[-- Attachment #2: test-full-text.html --]
[-- Type: text/html, Size: 5155 bytes --]

bug#22250: 25.0.50; Eww fails to break RTL paragraph

[Eli Zaretskii]

> From: Benjamin Riefenstahl <b.riefenstahl@turtle-trading.net>
> Cc: 22250@debbugs.gnu.org,  Lars Ingebrigtsen <larsi@gnus.org>
> Date: Mon, 28 Dec 2015 20:07:58 +0100
> 
> Now I have one, see attachment.  This, too, is fixed here by setting
> truncate-lines to nil in eww-mode.  The file can be loaded just through
> eww-open-file to exhibit the problem.

Thanks.  I think I see the problem: it's the auto-composition of
characters, in this case the Hebrew diacritics with the consonants.
Disable global-auto-composition-mode, and the problem goes away.  I
think shr.el's line-filling algorithm isn't ready for that, perhaps
because going back a character doesn't necessarily change point's
coordinates.

I will look into that, thanks for a clear test case.

bug#22250: 25.0.50; Eww fails to break RTL paragraph

[Benjamin Riefenstahl]

I see that you found something with that other file, that's good.  My
script-based test case contains no diacritics though, so it is still
another problem.  I have uploaded this now to my private webserver as
<https://odoacer.turtle-trading.net/abc-abc-abc-abc-abc-abc-abc-abc-abc-abc-abc-abc-abc-abc-test.html>.
This URL reproduces the problem for me after "G RET".

Eli Zaretskii writes:
> (Why is receiving a page from a server different from visiting the
> same page as a disk file?  The way shr works, this should produce
> exactly the same results, no?)

It gets worse, I set a breakpoint in eww-render yesterday.  At that
point in the code I understand that everything just works from a
temporary buffer containing the HTTP headers and the HTML.  I got the
error not depending on the buffer contents (IOW with the *same* contents
in the temp buffer), but depending on the length of the URL.  I thought
that the message "Loading ..." that is shown in the eww buffer might
trigger the problem.  So today I disabled that and tried my test case,
and it worked.  But than I tried my actual pages and came upon the case
that I just sent in the other post, where it still did not work.

> Failing that, can you describe what exactly do you see, or maybe step
> through the code in Edebug and tell which part(s) misbehave?

So I set a breakpoint on shr-vertical-motion and ran my script test
case.  Remember that this works the first time through but fails when
reloaded with "G RET".

In the good case, for the first line, point is at position 1,
shr-vertical-motion calls vertical-motion with (78 . 0) as parameter and
afterwards, point is at 94.  Which is a good place to break.

In the bad case, for the first line, everything looks the same,
vertical-motion gets called with the same parameter, but when it returns
point is at 161.  Which is not good.

Delving into the C code:

In the good case, it->w->hscroll is 0, in the bad case it->w->hscroll is
68.  Experimentation tells me that the interpretation of window-hscroll
(whether it refers to the left or the right margin) depends on
bidi-paragraph-direction, is that right?

Anyway, when I set bidi-paragraph-direction to left-to-right manually
before I press "G RET" than the problem does not occur.

Note that at the point when vertical-motion is called and gives
different answers, bidi-paragraph-direction is always right-to-left, so
it looks like some window parameter that depends on
bidi-paragraph-direction is cached somewhere?

bug#22250: 25.0.50; Eww fails to break RTL paragraph

[Eli Zaretskii]

> From: Benjamin Riefenstahl <b.riefenstahl@turtle-trading.net>
> Cc: 22250@debbugs.gnu.org,  larsi@gnus.org
> Date: Mon, 28 Dec 2015 22:23:23 +0100
> 
> I see that you found something with that other file, that's good.  My
> script-based test case contains no diacritics though, so it is still
> another problem.  I have uploaded this now to my private webserver as
> <https://odoacer.turtle-trading.net/abc-abc-abc-abc-abc-abc-abc-abc-abc-abc-abc-abc-abc-abc-test.html>.
> This URL reproduces the problem for me after "G RET".

Not for me, it doesn't.  I tried "G RET" quite a few times, it always
displays correctly.

> In the bad case, for the first line, everything looks the same,
> vertical-motion gets called with the same parameter, but when it returns
> point is at 161.  Which is not good.

What does window-hscroll return in each of these two cases?

> In the good case, it->w->hscroll is 0, in the bad case it->w->hscroll is
> 68.  Experimentation tells me that the interpretation of window-hscroll
> (whether it refers to the left or the right margin) depends on
> bidi-paragraph-direction, is that right?

Yes and no.  It depends on what you mean by "interpretation".

> Note that at the point when vertical-motion is called and gives
> different answers, bidi-paragraph-direction is always right-to-left, so
> it looks like some window parameter that depends on
> bidi-paragraph-direction is cached somewhere?

The value of bidi-paragraph-direction shouldn't matter when
bidi-display-reordering is nil (I've just went through the entire code
and didn't see any place where we use that value when
bidi-display-reordering is nil).  But just in case I missed something,
try bindings bidi-paragraph-direction to nil or left-to-right where I
bind bidi-display-reordering, and see if that helps.

Thanks.

P.S. I'm going to commit my patch, as it definitely improves things
and is clearly TRT to do (and I'm tired of stashing it ;-).

bug#22250: 25.0.50; Eww fails to break RTL paragraph

[Benjamin Riefenstahl]

Benjamin Riefenstahl writes:
>> I have uploaded this now to my private webserver as
>> <https://odoacer.turtle-trading.net/abc-abc-abc-abc-abc-abc-abc-abc-abc-abc-abc-abc-abc-abc-test.html>.
>> This URL reproduces the problem for me after "G RET".
>
Eli Zaretskii writes:
> Not for me, it doesn't.  I tried "G RET" quite a few times, it always
> displays correctly.

You're right it works now, since

  03dbfb9 * (eww-setup-buffer): Restore left-to-right defaults

Yesterday I had still been on the earlier f9d87dd.  Oh the joy of moving
targets ;-)

But that's ok, so this case and my script text case are solved in
practice.  I'm still concerned about the behaviour of vertical-motion in
this, though.

On practical level, there is still the matter of paragraphs using
diacritics.  Do you have an idea how to fix those?


For your other questions:

>> In the bad case, for the first line, everything looks the same,
>> vertical-motion gets called with the same parameter, but when it returns
>> point is at 161.  Which is not good.
>
> What does window-hscroll return in each of these two cases?

I checked that again with the commit before 03dbfb9.  I inserted a call
to `message' to make sure I got the right window.  It's 0 for the good
case, 67 for the bad case.  Basically the same as it->w->hscroll on the
C level.

>> In the good case, it->w->hscroll is 0, in the bad case it->w->hscroll is
>> 68.

>> Experimentation tells me that the interpretation of window-hscroll
>> (whether it refers to the left or the right margin) depends on
>> bidi-paragraph-direction, is that right?
>
> Yes and no.  It depends on what you mean by "interpretation".

I was starting from the doc on window.hscroll in window.h and the
docstring for window-hscroll.  The docstring says

  Return the number of columns by which WINDOW is scrolled from left
  margin.

But with bidi-paragraph-direction set to RTL window-hscroll works with
respect to the right margin.  I just verified again.

>> Note that at the point when vertical-motion is called and gives
>> different answers, bidi-paragraph-direction is always right-to-left, so
>> it looks like some window parameter that depends on
>> bidi-paragraph-direction is cached somewhere?
>
> The value of bidi-paragraph-direction shouldn't matter when
> bidi-display-reordering is nil (I've just went through the entire code
> and didn't see any place where we use that value when
> bidi-display-reordering is nil).  But just in case I missed something,
> try bindings bidi-paragraph-direction to nil or left-to-right where I
> bind bidi-display-reordering, and see if that helps.

With both variables set in shr-insert-document, I consistently get a
seemingly correctly wrapped but left justified (LTR) paragraph,
regardless which version I use, I tried in 5049827 (the one before
03dbfb9) and with the current 88e2de2.  This is with the above mentioned
URL.

> P.S. I'm going to commit my patch, as it definitely improves things
> and is clearly TRT to do (and I'm tired of stashing it ;-).

Got it.

bug#22250: 25.0.50; Eww fails to break RTL paragraph

[Eli Zaretskii]

> From: Benjamin Riefenstahl <b.riefenstahl@turtle-trading.net>
> Cc: 22250@debbugs.gnu.org,  larsi@gnus.org
> Date: Tue, 29 Dec 2015 21:55:24 +0100
> 
> Benjamin Riefenstahl writes:
> >> I have uploaded this now to my private webserver as
> >> <https://odoacer.turtle-trading.net/abc-abc-abc-abc-abc-abc-abc-abc-abc-abc-abc-abc-abc-abc-test.html>.
> >> This URL reproduces the problem for me after "G RET".
> >
> Eli Zaretskii writes:
> > Not for me, it doesn't.  I tried "G RET" quite a few times, it always
> > displays correctly.
> 
> You're right it works now, since
> 
>   03dbfb9 * (eww-setup-buffer): Restore left-to-right defaults
> 
> Yesterday I had still been on the earlier f9d87dd.  Oh the joy of moving
> targets ;-)
> 
> But that's ok, so this case and my script text case are solved in
> practice.  I'm still concerned about the behaviour of vertical-motion in
> this, though.
> 
> On practical level, there is still the matter of paragraphs using
> diacritics.

Is that the only remaining problem?  Reading the rest of your message,
I'm not sure.

> Do you have an idea how to fix those?

A general idea, yes.  I need to look into the details.

> I was starting from the doc on window.hscroll in window.h and the
> docstring for window-hscroll.  The docstring says
> 
>   Return the number of columns by which WINDOW is scrolled from left
>   margin.
> 
> But with bidi-paragraph-direction set to RTL window-hscroll works with
> respect to the right margin.  I just verified again.

That's a bug in the documentation, I will fix that.  Thanks for
pointing this out.

> >> Note that at the point when vertical-motion is called and gives
> >> different answers, bidi-paragraph-direction is always right-to-left, so
> >> it looks like some window parameter that depends on
> >> bidi-paragraph-direction is cached somewhere?
> >
> > The value of bidi-paragraph-direction shouldn't matter when
> > bidi-display-reordering is nil (I've just went through the entire code
> > and didn't see any place where we use that value when
> > bidi-display-reordering is nil).  But just in case I missed something,
> > try bindings bidi-paragraph-direction to nil or left-to-right where I
> > bind bidi-display-reordering, and see if that helps.
> 
> With both variables set in shr-insert-document, I consistently get a
> seemingly correctly wrapped but left justified (LTR) paragraph,
> regardless which version I use, I tried in 5049827 (the one before
> 03dbfb9) and with the current 88e2de2.  This is with the above mentioned
> URL.

Does this mean there's one more problem?  Or does this mean my
suggestion didn't work, and is not required, as the only remaining
issue is with composed characters?

Thanks.

bug#22250: 25.0.50; Eww fails to break RTL paragraph

[Benjamin Riefenstahl]

>> Benjamin Riefenstahl writes:
>> On practical level, there is still the matter of paragraphs using
>> diacritics.

Eli Zaretskii writes:
> Is that the only remaining problem?

At this point every problem in my application and test cases that I
still see involves diacritics.  And thank you (and Lars) for your
patience with that.

> Or does this mean my suggestion didn't work, and is not required,

It does fix the issue with my first test case, with the Latin text in an
RTL document.  I just checked and that one does not work if I revert
your change on HEAD.  Your change did not fix the issue with Hebrew text
without diacritics, that got fixed with Lars' commit 03dbfb9.

>> I'm still concerned about the behaviour of vertical-motion in this,
>> though.

> Reading the rest of your message, I'm not sure.

I am weary that there still are dragons with vertical-motion and/or
hscroll.  I don't like it that two situations behave differently,
although they have the same values in variables and the same window
configuration, point at in same place, and - from what I can see -
horizontal scrolling also at the same level (given that point is at bol,
hscroll *should* be 0, otherwise point would not be visible, right?).
Still window-hscroll gives a bogus value in the bad case.

>> With both variables set in shr-insert-document, I consistently get a
>> seemingly correctly wrapped but left justified (LTR) paragraph,
>> regardless which version I use, I tried in 5049827 (the one before
>> 03dbfb9) and with the current 88e2de2.  This is with the above
>> mentioned URL.

> Does this mean there's one more problem?

Not with my application and test cases.  But it seems to me, that
contrary to what you believed, bidi-paragraph-direction=nil still does
have an effect on the layout, even when bidi-display-reordering is nil.
It fixes the paragraph wrapping for Hebrew text without diacritics in
the version before Lars commit, while bidi-display-reordering alone does
not.  It also makes the paragraph LTR, but that does not surprise me as
much.

An insight that I just had - you maybe are aware of it already - is that
shr-tag-html, the function that sets bidi-paragraph-direction from the
<html> element, is called from with-in the body of shr-insert-document
(as a subroutine of the recursive shr-descend, basically).  That
explains why Lars' patch (resetting bidi-paragraph-direction at the eww
level each time before calling into shr) and setting
bidi-paragraph-direction in shr-insert-document have the same effect on
the wrapping.  It also explains why we get an LTR paragraph, when we
shadow the buffer-local variable in this way.  And it explains why I got
the problem only on "G RET".  The problem was triggered between the
start of the body of shr-insert-document and the call to shr-tag-html
when bidi-paragraph-direction is right-to-left between those two places.

bug#22250: 25.0.50; Eww fails to break RTL paragraph

[Eli Zaretskii]

> From: Benjamin Riefenstahl <b.riefenstahl@turtle-trading.net>
> Cc: 22250@debbugs.gnu.org,  larsi@gnus.org
> Date: Tue, 29 Dec 2015 23:33:33 +0100
> 
> >> Benjamin Riefenstahl writes:
> >> On practical level, there is still the matter of paragraphs using
> >> diacritics.
> 
> Eli Zaretskii writes:
> > Is that the only remaining problem?
> 
> At this point every problem in my application and test cases that I
> still see involves diacritics.

I fixed that case now, please try the latest emacs-25 branch.  It was
a very subtle problem with shr-vertical-motion (which actually
triggered a bug in vertical-motion).

> > Does this mean there's one more problem?
> 
> Not with my application and test cases.  But it seems to me, that
> contrary to what you believed, bidi-paragraph-direction=nil still does
> have an effect on the layout, even when bidi-display-reordering is nil.

I'll need a clear test case to look into this.  Every place in the
code I reviewed that references bidi-paragraph-direction does so only
if bidi-display-reordering is non-nil, but maybe I'm missing some
subtlety.

Thanks.

bug#22250: 25.0.50; Eww fails to break RTL paragraph

[Eli Zaretskii]

> Date: Tue, 29 Dec 2015 23:03:20 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: 22250@debbugs.gnu.org, larsi@gnus.org
> 
> > I was starting from the doc on window.hscroll in window.h and the
> > docstring for window-hscroll.  The docstring says
> > 
> >   Return the number of columns by which WINDOW is scrolled from left
> >   margin.
> > 
> > But with bidi-paragraph-direction set to RTL window-hscroll works with
> > respect to the right margin.  I just verified again.
> 
> That's a bug in the documentation, I will fix that.

Done.

bug#22250: 25.0.50; Eww fails to break RTL paragraph

[Benjamin Riefenstahl]

Eli Zaretskii writes:
>> At this point every problem in my application and test cases that I
>> still see involves diacritics.
>
> I fixed that case now, please try the latest emacs-25 branch.  It was
> a very subtle problem with shr-vertical-motion (which actually
> triggered a bug in vertical-motion).

That works.  Thanks, that's cool. 


> I'll need a clear test case to look into this.

Try the attached patch.  It reverts parts of Lars' fix and adds a debug
message to shr-vertical-motion.

For a base-line test, execute

  ./emacs -Q -nw --eval '(eww "https://odoacer.turtle-trading.net/abc-abc-abc-abc-abc-abc-abc-abc-abc-abc-abc-abc-abc-abc-test.html")'

Once the page is loaded, press "G RET".  This second load reproduces the
problem for me.  I get this protocol in the message buffer:

  Contacting host: odoacer.turtle-trading.net:443
  bpd: right-to-left pt: 1 hscroll: 0
  bpd: right-to-left pt: 97 hscroll: 0
  bpd: right-to-left pt: 193 hscroll: 0
  bpd: right-to-left pt: 289 hscroll: 0
  Contacting host: odoacer.turtle-trading.net:443
  bpd: right-to-left pt: 1 hscroll: 57
  bpd: right-to-left pt: 153 hscroll: 57
  bpd: right-to-left pt: 305 hscroll: 57

The first run is as I expected.  The second run has point at 1 and
hscroll at 57 (this is in a terminal, that's why the actual number is
different from before).  According to my logic that should not be
possible.  When the point is at 1, then hscroll should be 0 otherwise
point would not be visible.  Unless some intermediate state is
permissible.  But than shr could not rely on hscroll and therefore not
on vertical-motion.

Now as a second experiment, remove the ";" from bidi-paragraph-direction
in shr-insert-document.  Repeat the test.  Now the result should look
correct.  Somehow bidi-paragraph-direction does make a difference.

bug#22250: 25.0.50; Eww fails to break RTL paragraph

[Benjamin Riefenstahl]

[-- Attachment #1: Type: text/plain, Size: 77 bytes --]

Benjamin Riefenstahl writes:
> Try the attached patch.

I mean this one :-(


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: wrap.patch --]
[-- Type: text/x-diff, Size: 1569 bytes --]

diff --git a/lisp/net/eww.el b/lisp/net/eww.el
index e8fdc97..3d9d39c 100644
--- a/lisp/net/eww.el
+++ b/lisp/net/eww.el
@@ -401,7 +401,7 @@ eww-display-html
 	(source (and (null document)
 		     (buffer-substring (point) (point-max)))))
     (with-current-buffer buffer
-      (setq bidi-paragraph-direction 'left-to-right)
+      ;(setq bidi-paragraph-direction 'left-to-right)
       (plist-put eww-data :source source)
       (plist-put eww-data :dom document)
       (let ((inhibit-read-only t)
@@ -562,7 +562,7 @@ eww-setup-buffer
   (let ((inhibit-read-only t))
     (remove-overlays)
     (erase-buffer))
-  (setq bidi-paragraph-direction 'left-to-right)
+  ;(setq bidi-paragraph-direction 'left-to-right)
   (unless (eq major-mode 'eww-mode)
     (eww-mode)))
 
diff --git a/lisp/net/shr.el b/lisp/net/shr.el
index 330f7b5..4eee425 100644
--- a/lisp/net/shr.el
+++ b/lisp/net/shr.el
@@ -245,6 +245,7 @@ shr-insert-document
                                               (not (shr--have-one-fringe-p)))
                                          (* (frame-char-width) 2)
                                        0)))))
+        ;bidi-paragraph-direction
         bidi-display-reordering)
     (shr-descend dom)
     (shr-fill-lines start (point))
@@ -591,6 +592,10 @@ shr-fill-lines
       (goto-char (point-max)))))
 
 (defun shr-vertical-motion (column)
+  (message "bpd: %s pt: %d hscroll: %d"
+           bidi-paragraph-direction
+           (point)
+           (window-hscroll))
   (if (not shr-use-fonts)
       (move-to-column column)
     (unless (eolp)

bug#22250: 25.0.50; Eww fails to break RTL paragraph

[Eli Zaretskii]

> From: Benjamin Riefenstahl <b.riefenstahl@turtle-trading.net>
> Cc: 22250@debbugs.gnu.org,  larsi@gnus.org
> Date: Wed, 30 Dec 2015 21:22:06 +0100
> 
> > I'll need a clear test case to look into this.
> 
> Try the attached patch.  It reverts parts of Lars' fix and adds a debug
> message to shr-vertical-motion.
> 
> For a base-line test, execute
> 
>   ./emacs -Q -nw --eval '(eww "https://odoacer.turtle-trading.net/abc-abc-abc-abc-abc-abc-abc-abc-abc-abc-abc-abc-abc-abc-test.html")'
> 
> Once the page is loaded, press "G RET".  This second load reproduces the
> problem for me.  I get this protocol in the message buffer:
> 
>   Contacting host: odoacer.turtle-trading.net:443
>   bpd: right-to-left pt: 1 hscroll: 0
>   bpd: right-to-left pt: 97 hscroll: 0
>   bpd: right-to-left pt: 193 hscroll: 0
>   bpd: right-to-left pt: 289 hscroll: 0
>   Contacting host: odoacer.turtle-trading.net:443
>   bpd: right-to-left pt: 1 hscroll: 57
>   bpd: right-to-left pt: 153 hscroll: 57
>   bpd: right-to-left pt: 305 hscroll: 57
> 
> The first run is as I expected.  The second run has point at 1 and
> hscroll at 57 (this is in a terminal, that's why the actual number is
> different from before).  According to my logic that should not be
> possible.  When the point is at 1, then hscroll should be 0 otherwise
> point would not be visible.  Unless some intermediate state is
> permissible.  But than shr could not rely on hscroll and therefore not
> on vertical-motion.
> 
> Now as a second experiment, remove the ";" from bidi-paragraph-direction
> in shr-insert-document.  Repeat the test.  Now the result should look
> correct.  Somehow bidi-paragraph-direction does make a difference.

No, it's not bidi-paragraph-direction, at least not directly.  The
problem is that shr-fill-lines needs to start in a window that is not
scrolled horizontally, because otherwise vertical-motion will move to
a wrong place (it interprets the column in its argument to be relative
to its left edge).  When EWW doesn't clear the previous value of
bidi-paragraph-direction, the message "Loading URL ..." that is
displayed when you type "G RET" is inserted into a buffer with a
right-to-left paragraph direction, and bidi-display-reordering was not
yet set to nil, so inserting that message with such a long URL causes
the window to auto-hscroll.  So when shr-insert-document is called,
and resets bidi-display-reordering, the window is already hscrolled,
and filling lines misbehaves.

I fixed that now by forcing zero hscroll on the window before the
line-filling starts.

I guess we can now close this bug?

Thanks.

bug#22250: 25.0.50; Eww fails to break RTL paragraph

[Benjamin Riefenstahl]

Eli Zaretskii writes:
> So when shr-insert-document is called, and resets
> bidi-display-reordering, the window is already hscrolled, and filling
> lines misbehaves.
>
> I fixed that now by forcing zero hscroll on the window before the
> line-filling starts.

O.k.

> I guess we can now close this bug?

Sure, thanks a lot.

bug#22250: 25.0.50; Eww fails to break RTL paragraph

[Eli Zaretskii]

> From: Benjamin Riefenstahl <b.riefenstahl@turtle-trading.net>
> Cc: 22250@debbugs.gnu.org,  larsi@gnus.org
> Date: Thu, 31 Dec 2015 19:10:11 +0100
> 
> > I guess we can now close this bug?
> 
> Sure, thanks a lot.

Done, thanks.