Bug 1825

Comparing kdoctool files between my Jan. 28 and Mar. 4 tdelibs packages show nominal changes only, mostly entity renames. That tends to imply the regression might be in the generation of the html files rather than the rendering.

The problem is Trinity HTML rendering engine.

I performed some nominal tests that seem to eliminate the html generation during compiling.

Try the following:

* cd to a temporary working directly, such as /tmp

* Run the following:

meinproc --stylesheet /opt/trinity/share/apps/ksgmltools2/customization/tde-chunk.xsl /home/public/builds/slackware/trinity/zz_src_trinity_git/main/tdebase/doc/userguide/index.docbook

This command will create a file named index.html in the current working directory. The file is not compressed (cached) as normally done with the help files, but is the same HTML file. This command command simulates the html generation during compiling. The tde-chunk.xsl file is the same xsl processor file used during compilation.

Open the HTML file in Firefox. The table of contents indentation is correct and as expected.

In the next test, I unarchived /opt/trinity/share/doc/tde/HTML/en/khelpcenter/userguide/index.cache.bz2 to a file named index.cache. I then edited the file and removed two elements:

At the very beginning of the file: <FILENAME filename="index.html">
At the very end of the file: </FILENAME>

I saved the file. With those two deletions the file becomes an html file. When viewed in Firefox the table of contents indentation is correct and as expected.

As the contents of the help handbook are html files, and these tests indicate a normal browser indents the table of contents correctly, that indicates the files themselves and the associated css files are okay. This indicates the problem is rendering.

As a final test, I opened both test files in konqueror. I then saw the same rendering problem with the indentation as I do in the help handbook. Therefore the problem seems to be the Trinity HTML rendering engine.

Based upon my testing of previous packages sets from Jan. 28 and Mar. 4, the 2013-01-27 system wide set of KHTML->TDEHTML rename patches seem suspect, as well as commit 1b57c467 from 2013-02-07, "Regenerate TDEHTML flex/bison/gperf generated files."

> The problem is Trinity HTML rendering engine.

Well done Darrell, I confirm the same behavior. Using Firefox and Chrome the indentation is ok, while using Konqueror and the TDE help system, the indentation is incorrect.

Darrell, would it be possible for you to compare one if the html files from the two build sets (Jan. 28 and Mar. 4 2013.)?
If they are identical/very similar, the problem is a HTML rendering problem.
If they are different, the problem could be a html file generation problem.

I suspect the first case, since Firefox and Chrome are rendering correctly. But given they are much more recent compare to Konqueror they may be doing some automatic corrections that result in correct rendering. Just worth checking this as well since it should not take that long.

>would it be possible for you to compare one of the html files
>from the two build sets (Jan. 28 and Mar. 4 2013.)?
The handbook files are almost identical in both package sets. View changes between those dates to the user guide here:

http://git.trinitydesktop.org/cgit/tdebase/log/doc/userguide/index.docbook

I can't read! The docbooks are identical between the two package sets. The changes made to the user guide between the two dates were in 2012, not 2013.

Likewise, I sampled the amarok handbook in Firefox and Konqueror. Same offset indentation results.

I suspect commit 1b57c467 more than the renaming patches. The renaming patches would affect more than just a single obscure rendering issue. But I'm just guessing.

This rendering problem affects more than the table of contents. Bullet lists do not indent correctly.

Bumping to critical. This is a horrible bug and reflects badly on the project.

Adding to the R14 etherpad.

It seems that it is the same problem as reported in bug 1692.

>It seems that it is the same problem as reported in bug 1692.
Yes, seems so.

I'm guessing the problem is parsing any css list, not just bullet lists.

BTW, KDE4 konqueror renders correctly.

How was commit 1b57c467 generated?

(In reply to comment #12)
> How was commit 1b57c467 generated

The problem is not commit 1b57c467: I rebuilt tdelibs with that commit reversed, and things actually got even worse.
The indentation is still wrong, but now also the banners at the top and bottom of the help pages are broken :(

Nevertheless I also would like to know how the files modified in that commit were actually modified (manually??)

Darrell,
can you attached the following file

tdelibs/doc/common/tde-default.css
(or alternatively /opt/trinity/share/doc/tde/HTML/en/common/tde-default.css)

from the two sets of packages that behaves differently (Jan 28 and Mar 4)?
I expect them to be different. This should help leading us to the problem.

Next week I am going to work on this bug, since I have grown sick of the badly indented handbooks.

>The problem is not commit 1b57c467: I rebuilt tdelibs with that commit
>reversed, and things actually got even worse.
I tried that several days ago. Same results. :)

>Next week I am going to work on this bug, since I have grown sick of the badly
>indented handbooks.
The outdents are disruptive and make reading a challenge. I have struggled with the outdents as I test the help handbooks.

>tdelibs/doc/common/tde-default.css
I don't have source tarballs, only the final packages. Regardless, the css files between the two tdelibs package sets are identical. Other than the kde->tde file renaming after Mar.4 (for example, kde-default.css->tde-default.css), the files all are identical to what is now in git. The file renaming occurred after Mar.4 and do not play a role. Look at what is in git now and that is what is in both of these package sets.

I diffed the entire /opt/trinity/share/doc between the two tdelibs packages and the only difference is kspell->tdespell.

Other than the changes noted in comment 3, there were other renames such as kio->tdeio. Yet this problem seems to be a rendering issue such as the rendering engine not interpreting something correctly.

(In reply to comment #16)
> Regardless, the css files between the two tdelibs package sets are identical.
Auch! That surprises me a little. In the css file there is no definition of the sect2 class for example....
Yesterday I just had a quick look at this, next week I will have to dig deeper so.

>That surprises me a little. In the css file there is no definition of the
>sect2 class for example
I noticed that too. I think I mentioned that in the mail list. The curious part is the css indenting worked fine before, despite no sect2 class being defined. Now that I wrote that, this could be another example of a latent bug that got exposed by the series of patches. Perhaps now we do need to add a sect2 class to the css?

A challenge with my previous comment is adding a sect2/sect3/sect4 class in the css does not resolve what is probably the same problem in bug 1692.

Created attachment 1895 [details]
test file

This is related to bad rendering of lists in Konqueror. You can try the attached file in Konqueror and Firefox to see the difference.
Working on it :)

Darrell,
using the test file I attached in the previous comment, would it be possible for you to attach a screenshot of how that is correctly rendered in Konqueror using the package from Jan 28 2013? That would serve as comparison. I don't need the screenshot from March 4 since I can get wrong rendering from the current version.
Thanks

Created attachment 1896 [details]
Screen capture of testDL in R14-2013-01-28

(In reply to comment #22)
> Screen capture of testDL in R14-2013-01-28
Thanks Darrell, I appreciate the time you put in for this.
I am (slowly) working on this. Unless I am very lucky, I think it will take a while to find where the problem is, so be patient :)

Created attachment 1901 [details]
small test page

Some progress :) I got markers and numbers to come out on unordered lists and ordered lists respectively. It seems as if there was a latent bug, which for some reasons (still unknown) has been exposed. The file tdehtml/rendering/render_lists.h has a function "listPositionInside()" which is supposed to say when the list item is inside a ordered/unordered list.

bool listPositionInside() const
{ return !m_listItem->m_insideList || style()->listStylePosition() == INSIDE; }

where m_insideList is a pointer to the parent element if the item is inside a list, otherwise it is null.
The first part of the OR should not be negated as in 

bool listPositionInside() const
{ return m_listItem->m_insideList || style()->listStylePosition() == INSIDE; }

With this little modification, markers and numbers have come back for list items inside a list. For items outside a list instead, the markers have disappeared.

Darrell, I need again your help to check something on the Jan 28 package. Can you post a screenshot of the attached html file as rendered one year ago? Thanks.

Indentation of item is still a problem though. Next thing to look for.

Created attachment 1902 [details]
test file on Konqueror with the change explained in the previous comment

(In reply to comment #24)
Ok, forget what I said about the bug in "listPositionInside()" :(
Items outside of the list should still have a dot marker, and that was probably the function of "!m_listItem->m_insideList" in the first part of the OR.
The bug must be somewhere else, and related to the "style" in the second part of the OR.

Darrell, the screenshot would be useful nevertheless, since it would show element inside and outside of lists and also nested lists..

Created attachment 1903 [details]
Screen capture of testDLsmall in R14-2013-01-28

I am working hard on this bug report, but so far I have not been able to find where the problem is. Nevertheless I thought about giving an update.
I am in the process of tracking down the git commit that introduced the bug. So far I managed to go back to Jan 27 2013, and the bug was already there.

In detail, I compiled a base system (from tqt3 to tdebase) based on the following commits (with the necessary manual fixes to get things to compile and to install correctly):
tqt3         - Dec 13 03:07:33 2013 - ca9d4d7d227386aba0f30bdacee1c5aa0a053204
tqtinterface - Dec 11 01:36:20 2013 - b67c2e559a69620288450b2140152cd231d2dcfd
arts         - Jan 27 00:56:46 2013 - f71f47fd438b270247fd2a0bc5c54ea7d963486a
dbus-tqt     - Jan 27 04:13:22 2013 - 8297fba112d8eabbc2e809f03ab1d1cca0e81afd
dbus-1-tqt   - Jan 27 04:12:01 2013 - c02b777653c963a7ca091f96fb851d826cce73d8
tqca-tls     - Jul 24 23:18:23 2013 - 49aac943cca03cf2b5fe37cc4c034e2d022f03c0
libart-lgpl  - Apr 19 20:07:58 2012 - 2e6e0a12e1d0962109fa2454b72bb0b9e6ae609e
avahi-tqt    - Jan 26 13:14:10 2013 - 567a46128cddd5488804998854258dda5703721c
libcaldav    - Mar 21 20:49:54 2012 - 03705c223966657a6cca466fc96188a2b1621c39
libcarddav   - Jan 26 13:14:13 2013 - c3b3bbf51ae2fd1acb3421b422433602580ae4fe
tdelibs      - Jan 27 13:41:53 2013 - a3e01ba75c1f4886f3d2f2abeeb2715f39957974
tdebase      - Jan 27 15:11:21 2013 - 472156a41b1348c714986c772759ad950fffbe75

I tested such packages in a clean Debian/Jessie netinstall, no other DE installed together with TDE. As said the bug was already there.
I will continue to go further back, since Darrell has a working set of packages from Jan 28 which does not have the problem.

Darrell, a questions for you. Are the sets from Jan 28 and Mar 4 tested on the same machine or on two different machines? And what about the profile used?

>I compiled a base system (from tqt3 to tdebase) based on the following commits
How do you do that?! Often I have wanted to compile a package set that reverts to an previous date.

>Are the sets from Jan 28 and Mar 4 tested on the same machine or on two
>different machines? And what about the profile used?
Same machine, same profile. Originally I tested with full package sets, but now I have only the core packages installed on the test machine (everything to tdebase).

I am not surprised that you found the bug present before my "good" date. Bear in mind that my package set date of Jan. 28 does not mean I updated my local repo on that date as well. That package set could be from a local repo update of up to several days earlier.

>>I compiled a base system (from tqt3 to tdebase) based on the following commits
>How do you do that?! 
A) Use "git checkout commit_hash" on the submodule you want to compile (you can use the commit hashes in the list in my previous post as reference). This will revert your submodule sources to that particular commit.

B) Then compile as usual. Keep in mind two things:
1) at that time there was a lot of renaming involved, so sometimes packages are broken and do not build. In that case you need to manually fix things. In my case, tdelibs and tdebase needed quite a lot of manual fixes.
2) from one year ago, required packages may have changed, so you may encounter bugs that have been fixed in the present git repo but there were not there one year ago. If a package doesn't build, that could be one of the reasons.

C) Once done, "git checkout master" brings the submodule back to present day.


>> Are the sets from Jan 28 and Mar 4 tested on the same machine ...
> Same machine, same profile.
Thanks

Note that the problem is not just padding in the lists, but even in counting the size of the group box (pictures of Monast in bug 1692). I am afraid that the problem was caused by the renaming khtml => tdehtml. Maybe somewhere substring by number of characters.

(In reply to comment #31)
Thanks Slavek, I hadn't noticed the problem with the groupbox.
Would you be able to provide a small stripped-down html sample file from the asterisk website (no need for specific contents, just the group box layout) or a website that I can use for testing it? I tried using fieldset to replicate that, but it is rendered correctly.

Good news for the list item problem. I have been able to restrict the search to the following interval and packages:

- tdelibs: from Jan 12 04:26:39 2013 - 4c9ff70f (not included) 
           to   Jan 27 13:41:53 2013 - a3e01ba7 (included)
- tdebase: from Jan 12 16:36:04 2013 - 18f6f27f (not included) 
           to   Jan 27 15:11:21 2013 - 472156a4 (included)

There are 30 patches in tdelibs and 25 patches in tdebase in that period of time, so one (or more) of them introduced the bug for the list items, and possibly also for the group box.

I managed to truck down the problem to the rename of khtml to tdehtml in tdelibs/tdebase, as we already suspected. 
The patch dfe28985 in tdelibs is *only* 613000 lines long, mostly due to the amount of renaming done (not only khtml but also other folders).
I verified that the single khtml -> tdehtml rename is responsible for the problem.
The next step is to look into the khtml -> tdehtml rename and see what could be responsible for the bug.
In any case I have both a good and a buggy environment, so I can do comparison test/debug work.
Be patient :)

Created attachment 1916 [details]
tdelibs CSS -khtml extension patch

After a lot of hard work, I finally found the problem. 
The problem is caused by the rename of the -khtml-* properties in the CSS style definitions.
The -khtml-* are proprietary extensions used by Konqueror (and Safary until version 2) to CSS files. Renaming then to -tdehtml-* make all of them useless, resulting in bad rendering of html pages that uses them. Since the default html4.css makes use of them (for example '-khtml-padding-start' for the list objects), a lot of pages are affected.
I am going to spend a little more time on this for my own knowledge (I want to find out where those -khtml-* are actually "broken down", because it is somehow hidden in the code).

Some web developers still use the -khtml-* for compatibility with Konqueror, but many don't do that anymore (just google "CSS -khtml" if you are interested).
No developer will ever include a -tdehtml-* in their webpage.
So the correct fix is to revert those -tdehtml-* properties to -khtml-* ones. 

NOTE: *** the patch is untested *** 
I need to do a full rebuild of a base system tomorrow, but I am posting the patch in case you want to test while I am sleeping.
The equivalent patch on the tdelibs of 2013/01/26 fixed the problem correctly.

PS: funny thing, when I was looking at this bug at the very beginning I had a feeling that those -khtml-* renames were actually the problem. But I didn't know enough to focus on that only, and then it took a lot of tdelibs/tdebase buildings to prove that.

I looked into the source code of MonAst and are there to find things like: navigator.userAgent.indexOf ('KHTML').

It seems to be a good idea revert also identification.

Created attachment 1917 [details]
tdelibs : Restore KHTML in http agent identification

Attached patch restores KHTML in the Konquror browser / http agent identification.

I tested both patches by rebuilding tdelibs and tdebase.

The table of contents indent problem is fixed but introduced new problems.

The header banner is half-page.

The handbook header "T" logo image appears underneath the header banner image.

The Prev and Next links at the top and bottom of the pages do not render in the correct locations. The links now are both on the left side of the page with Next underneath Prev.

Seems to be universal.

(In reply to comment #37)
I saw this problem (or something similar) when I reverted commit 1b57c467.
Probably commit 1b57c467 fixed the problems introduces by the -khtml-* rename.
Now that we have reverted -tdehtml-* to -khtml-*, we probably need to revert also commit 1b57c467.

I will test today and update

Created attachment 1919 [details]
new cumulative tdelibs patch

This patch is a cumulative patch that includes:
1) my previous -tdehtml-* -> -html-* rename
2) Slavek's patch for browser identification
3) reversion of commit 1b57c467

I tested on my system and with this patch, UL, OL and DL are correctly displayed and the handbooks are once again displayed correctly.

Please test. Let me know when I can push

Created attachment 1920 [details]
Help handbook main page snapshot (attachment corrupted)

Created attachment 1921 [details]
Kate index page snapshot (attachment corrupted)

The latest patch seems to have resolved the problems with the help handbook indentation and header display.

Hopefully the patch resolves bug 1692 too. We'll have to wait for Slavek's report.

A side comment: can we fix the amount of indentation? The amount of indention is excessive. Look at the user guide table of contents. Probably a *.css issue rather than the rendering engine.

I tested the patch. All errors listed here and in bug 1692 are resolved! The contents of handbooks is fine, Munin is fine, MonAst is fine, everything is fine :)

One note: For easier tracking version numbers in files syntax highlighting between original Kate files and TDE I would suggest adding "patch" version number. For example - css: instead of change from 1.01 to 1.02 I would suggest change to 1.01.1. The same way has been used in commit 50d58f6f: Update kate syntax highlight file for POV-Ray.

Pushed to GIT as commit 6b4be5f.

> One note: For easier tracking version numbers in files syntax highlighting
> between original Kate files and TDE I would suggest adding "patch" version
> number.
I see your point. I will update all the necessary files (about 200 or so) when I do the update at the end of this month (about 2-3 weeks later).

> A side comment: can we fix the amount of indentation?
That's a tricky thing to do. I did some tests modifying html4.css in different ways, but I always ended up screwing something else up.
We should probably look at modifying just how the main index file is generated.

Looks like this monster introduced its own regression. :(

In the current kicker handbook ("right-click" the panel, select Help->TDE Panel Handbook), there is a section where the Prev/Next links are broken.

In the kicker handbook table of contents, select

"3. Kicker basic"

When the page loads, notice the Prev/Next links are dead.

I tested a package set from Feb. 6 that did not include commit 6b4be5f and the links work as expected. I tested a package set from Feb. 7 when the patch was attached to this bug report but not yet committed to git and the Prev/Next links fail, just as with my present git package set.

I have not yet looked at other help handbooks.

(In reply to comment #45)
I will check this, but I think it is more likely that the problem was introduced by a separate commit. The fix for this bug should only affect how some elements are visualized, not how links are interpreted. Otherwise I would expect all prev/next links to be broken, not just one in Kicker.
Was the uncommitted patch the only code difference between the two packages that you tested? I am asking because there were a lot of commits on Feb 6.

I will update after I check.

>(In reply to comment #45)
>In the kicker handbook table of contents, select
>"3. Kicker basic"
>When the page loads, notice the Prev/Next links are dead.

Darrell, I followed your instructions. 
At first the Prev/Next links all worked fine on my system.
As a matter of fact I navigated the whole Kicker handbook from the index to the last page and back to the index using the Next/Prev links on the top of the page, and also repeated using the links at the bottom.
No problem whatsoever, no dead links, no wrong "jumps".
I did a full TDE rebuild/installation three days ago.

I tried the same in Firefox, no problem at all.

I tried the same in Konqueror and found an interesting behavior: if the width of the page is big enough, the links work. If the page is not wide enough, the links are dead. But only on page "3. Kicker basic".
I repeated the test using the Helpbook but using a narrower page, the problem appeared.
I repeated the test using Firefox, no problem at all.

I repeated the same test using Kate's handbook and a very small page width (Konqueror, Helpbook, Firefox): no problem at all.

Wait, even more strange: if the page is very narrow or wide enough, no problem. If it is within a given range, I have dead links *only* on page "3. Kicker basic".

There must be something in page "3. Kicker basic" that upset tdehtml, probably exposed when the -tdehtml-* where correctly renamed to -khtml-*.

Would be interesting if you could do the same test using very old packages (before 2013) or even better 3.5.10, to understand if it is something that we introduced or just something that has been there for a long time.

Darrell,
I rebuilt tdelibs with commit 6b4be5f (bug 1825) reverted and tested again, but the problem was there already.

So there are two possibilities:
1) when you tested the packages from Feb 6 and Feb 7 you used different window sizes. In such case this would explain the different behavior you observed and the bug was there from before.

2) when you tested the packages from Feb 6 and Feb 7 you used the same window sizes. In such case the problem was introduced in a different commit around Feb 6.

Please try again and let me know which of the two cases we are talking about.

In any case though, I am closing this bug and opening a new regression bug 1950 for the Kicker handbook, since the problem was not caused by this patch.