litehtml 0.7/0.8 break qttools-6.5.0

[0-wiz-0]

Some of the changes in litehtml 0.7 break qttools-6.5.0.
Here's a build log, the errors start with

src/assistant/qlitehtml/src/container_qpainter_p.h:61:52: error: 'tchar_t' in namespace 'litehtml' does not name a type
   61 |     litehtml::uint_ptr create_font(const litehtml::tchar_t *faceName,
      |                                                    ^~~~~~~

build.log

[0-wiz-0]

See also https://bugreports.qt.io/browse/QTCREATORBUG-29169

[tordex]

Interface of litehtml was changed. All strings are now have type char* or std::string to make litehtml more clean.
So qttools should be updated to use the latest litehtml.

[cirospaciari]

Interface of litehtml was changed. All strings are now have type char* or std::string to make litehtml more clean. So qttools should be updated to use the latest litehtml.

this makes my life way easier when using litehtml.

[e4z9]

Here is a WIP patch for adapting the Qt wrapper for litehtml 0.8 https://codereview.qt-project.org/c/playground/qlitehtml/+/479103.

Unfortunately element::get_position() and document->root()->get_element_by_point(...) were removed, and I don't see a way to get the render_item for an element. We used this information for a few things: Keeping the current top element at the top when resizing the view, and for selection for copy & paste and searching. The code that did that is currently still commented out in the patch above.

[e4z9]

It looks like the render_item has a backpointer to the src_el(), it would be great if we had it the other way round, element->render_item, too

[dewhisna]

@e4z9 -- I didn't see this WIP until I had already done the same thing here locally to see if I could get it working with the latest litehtml. What seems to be missing is a root_render helper on the document object that returns a pointer from m_root_render like there is for m_root. Then external clients can call it just like they are doing here on the document object for the same reasons:

litehtml/src/document.cpp

Line 464 in 32e294f

element::ptr over_el = m_root_render->get_element_by_point(x, y, client_x, client_y);

But yeah, that was the showstopper I hit as well. Except instead of disabling that on the qttools side, I temporarily patched litehtml until this could get decided.

I guess I need to compare our two WIPs to see if we arrived at the same code. The char and string type changes were trivial. Then I had to play hunt-the-function to find where various things moved between the element and css objects. The most cumbersome change was the change from std::vector to std::list, as all of the index logic on the Qt code side needed reworking to allow for a less-random access container. But then I hit this showstopper and not finding anyway to call get_element_by_point.

I'm eager to see where all of this ends up.

[dewhisna]

@e4z9 -- looks like we came up with nearly the same things. One thing different is that I was working against the current HEAD of litehtml instead of the v0.8 tag, and elements_vector has been replaced by elements_list there. Plus functions like is_root, get_children_count, and get_child are gone. So I've already dealt with those changes too.

On your comment about element::get_position(), what's the difference between get_position and get_placement? Maybe I incorrectly assumed I could just change the code to use get_placement in those cases as DocumentContainer::withFixedElementPosition uses. But that might be my lack of understanding the difference between the original get_position and get_placement.

And since is_root is gone now too, I think the if (!current->is_root()) can be replaced with if (current->parent()). And the get_children_count and get_child can be replaced by function calls on the container returned now by children().

The one key place we disagreed on was in the new loop in DocumentContainerPrivate::draw_background. When bg.is_root returns 'true', you are breaking out of the loop, but I used continue because I assumed we needed to process the rest of the objects in the supplied vector rather than bailing out. Do we not?

And in DocumentContainerPrivate::buildIndex(), which you've commented out, isn't that just a change from current->is_visible() to current->css().get_visibility()?

Apart from having to stub in the missing get_element_by_point into my local litehtml checkout, what I have seems to be working, both in compiling and in function.

I've attached (zipped due to attachment type limits) my version of container_qpainter.cpp so you can compare with your Qt patch: container_qpainter.zip

[e4z9]

I think the main difference is that "position" seems to be the position in the parent, and "placement" is relative to the document.

I've updated https://codereview.qt-project.org/c/playground/qlitehtml/+/479103 with workarounds for the missing element::is_visible and element::get_element_by_point, where with the former one might return true for some elements that it shouldn't (render_item::is_visible takes its "skip" property into account, whatever that is), and the latter one is still wrong for scrolled views (and ignores all the fancy conditions, zordering and things)

[e4z9]

So, I've finished the patch updating the Qt backend to litehtml v0.9 . Not sure if it works correctly in all cases (z-ordering and fixed position elements are ignored for selection/hover). But generally speaking I think that this issue can be closed.

[dewhisna]

@e4z9 -- don't hold me to it, as this is a busy time for me, but I will try next week to pull your qlitehtml patch and do some testing of it here with my use case.

[dewhisna]

@e4z9 -- I managed to get ahead in my testing work and you have a thumbs up from me on your Qt backend patch at the current Patchset 11, 7071977bd53c0e5f0bc5efba55505bb898ce8136 hash, in that qt-project repo that you linked above. I only found one issue when applying it to my use case and moving to litehtml v0.9, but I don't see anyway it can be caused by the Qt wrapper.

The issue seems to be litehtml itself not correctly computing the overall document size and seems related to the margins. The Qt wrapper is telling litehtml to render the sizing for the current viewport, including removing of the vertical scrollbar, etc. But litehtml is computing an overall document size that's larger than this maximum size. The master.css has an 8px margin on body and if I override that to 0px, the problem seems to mostly be solved, except for perhaps some of the flex wrapping computations for the reverse direction.

I can't find anywhere in the Qt wrapper that's supplying any other sizing constraint to litehtml other than through that call to render() and the wrapper is correctly computing the viewport sizing there. And if I step into litehtml code in the debugger there, it computes a document size that's bigger than that max. So, I believe it's a litehtml bug that needs to be filled separately. The previous v0.6 litehtml implementation doesn't seem to exhibit this, so it seems to have manifested between v0.6 and v0.9.

For now, I'm going to work around that quirk in my application by just setting the margin to zero, as having it fit the viewport correctly is less annoying than losing the margin.

But as far as your Qt backend wrapper updates, it seems to work just fine with v0.9 of litehtml. And I concur that this issue can be closed.

[e4z9]

not correctly computing the overall document size

Right, and the top navigation bar of the Qt documentation is too wide as well. It wasn't fixed for me just by removing the margin in the body in master.css, but after an update of the master css to the current version in litehtml fixes the issues for me: https://codereview.qt-project.org/c/playground/qlitehtml/+/544585

[dewhisna]

@e4z9 -- nice catch! I didn't think to compare the master.css with the current litehtml version. Your latest changes to QLiteHtmlWidget fixed my issue as well. I was able to drop my workaround with changing the margin and everything is working as it should now.

The only thing I'm currently waiting on, totally unrelated to this issue, is the dir attribute for full RTL layout support. I'm mostly getting by without it, thanks to the better support for flex layout in these newer litehtml versions which support row-reverse, but there's a few minor cases where it breaks without having the dir attribute. With these changes and getting to litehtml v0.9, I'm at 99% of a fully working solution.