Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Better support for intrinsics, table, vertical-align, and display #1306

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Sub6Resources
Copy link
Owner

No description provided.

@codecov
Copy link

codecov bot commented Jun 7, 2023

Codecov Report

Attention: Patch coverage is 44.94382% with 49 lines in your changes missing coverage. Please review.

Project coverage is 63.96%. Comparing base (79ec194) to head (90550dd).

Files Patch % Lines
lib/src/css_box_widget.dart 23.91% 35 Missing ⚠️
lib/src/style/margin.dart 0.00% 7 Missing ⚠️
lib/src/style.dart 50.00% 5 Missing ⚠️
lib/src/builtins/styled_element_builtin.dart 77.77% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1306      +/-   ##
==========================================
- Coverage   64.91%   63.96%   -0.95%     
==========================================
  Files          38       39       +1     
  Lines        2981     3022      +41     
==========================================
- Hits         1935     1933       -2     
- Misses       1046     1089      +43     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

context.styledElement!.style.display == Display.inlineBlock) &&
final style = context.styledElement!.style;
final display = style.display ?? Display.inline;
if (display.displayListItem ||
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can remove this

],
style: Style(),
),
child: SizedBox.expand(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This now allows nested tables I think, which is great! Can we still use a wrapper TagWrapExtension to make the table horizontally scrollable?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question, I didn't check that in my testing. I'll look over that again and let you know!

@Sembauke
Copy link

Sembauke commented Jun 7, 2023

What does this pr do?

@Sub6Resources
Copy link
Owner Author

Sub6Resources commented Jun 7, 2023

Sorry @Sembauke I opened this right as a flight I was waiting for started boarding, so it didn't include any details and still needs some tests written.

Mainly, this fixes #1165 by properly interpreting the CSS vertical-align property on all elements. Most elements, block-level elements in particular, don't need to align with the text baseline, so they will default to PlaceholderAlignment.bottom, which has no issues in an intrinsically sized context. It also adds a try-catch that keeps dry layout from failing and instead prints a more developer-friendly message to the console letting them know that their html in an intrinsic context may have layout bugs that can be fixed by explicitly setting vertical-align to bottom. So far I've only seen this triggered by images within a table, but I'm sure there are many other cases this will come up in, especially within the intrinsic context of tables.

This PR also expands the Display enum to use a more css-like approach under the hood and adds the full set of values, even if they aren't supported by the core flutter_html so that extensions like flutter_html_table can utilize display: table; and other related properties. This means that extension authors can more easily implement things like grid and flex display types without needing to parse the CSS on their own.

@Sub6Resources Sub6Resources changed the title Fix/intrinsics and table fix: Better support for intrinsics, table, vertical-align, and display Jun 7, 2023
@droplet-js
Copy link

Any news update? I have same issue, when i use Html Widget as CupertinoAlertDialog content.

@frankvollebregt
Copy link

For me, the current beta version doesn't correctly calculate the intrinsic height, so I'm using a dependency override from this branch (which works great for my needs!). When is this expected to be merged into a (pre)release?

@omar3alaa
Copy link

omar3alaa commented Aug 3, 2023

Hey! Any ETA when this gonna be merged and released? @Sub6Resources

@EtcGonza
Copy link

EtcGonza commented Aug 8, 2023

Greetings! Any update? I have the same issue...

@omar3alaa
Copy link

Hey @Sub6Resources! Really appreciate your efforts on this PR, it helps a lot to fix an existing issue for a lot of the plugin's users. Do we have any expected timeline for it to be released as we blocked by it for releasing a new feature for our company?

@ManuEspesoJbs
Copy link

No ETA? I'm facing same problem

@Sub6Resources Sub6Resources self-assigned this Dec 18, 2023
@Sub6Resources Sub6Resources added this to the 3.0.0 milestone Dec 18, 2023
@shovel-kun
Copy link

This PR causes a regression in nested list alignment in the following HTML snippet.

<div> <ol> <li> <ul> <li>to read</li> </ul> <div> <div> <div> <div> ミルトンの <ruby><rt>さく</rt> </ruby> <ruby><rt>ひん</rt> </ruby><span>読んだ</span>ことがありますか。 </div> <div>Have you ever read Milton's works? <span>[1]</span></div> </div> </div> </div> </li> <li> <ul> <li>to recite (e.g. a sutra)</li> <li>to chant</li> </ul> </li> <li> <ul> <li>to predict</li> <li>to guess</li> <li>to forecast</li> <li>to read (someone's thoughts)</li> <li>to see (e.g. into someone's heart)</li> <li>to divine</li> </ul> <div> <div> <div> <div> <ruby><rt>かの</rt> </ruby> <ruby><rt>じょ</rt> </ruby><ruby><rt>こころ</rt> </ruby><ruby><rt>うご</rt> </ruby>きを <span>読む</span>ことさえできなかった。 </div> <div> I could not even make a guess at the working of her mind. <span>[2]</span> </div> </div> </div> </div> </li> <li> <ul> <li>to pronounce</li> <li>to read (e.g. a kanji)</li> </ul> </li> <li> <ul> <li>to decipher</li> <li>to read (a meter, graph, music, etc.)</li> <li>to tell (the time)</li> </ul> <div> <div> <div> <div> <ruby><rt>わたし</rt> </ruby><ruby><rt>あたま</rt> </ruby><ruby><rt>がく</rt> </ruby> <ruby><rt></rt> </ruby><span>読む</span>ようにはよく <ruby><rt></rt> </ruby>らされて <ruby><rt></rt> </ruby>りますが、どうも <ruby><rt>あん</rt> </ruby> <ruby><rt>ごう</rt> </ruby><span>読む</span>には <ruby><rt>てき</rt> </ruby>しません。 </div> <div> My mind is quite accustomed to reading music, but deciphering secret codes is not its forte. <span>[3]</span> </div> </div> </div> </div> </li> <li> <ul> <li>to count</li> <li>to estimate</li> </ul> <div> <div> <div>Note</div> <div>now mostly used in idioms</div> </div> <div> <div> <div> <span>See:</span> <a> <span >さばを <ruby><rt></rt> </ruby></span> </a> </div> <div> to manipulate figures to one's advantage; to count wrongly on purpose; to inflate or deflate one's age </div> </div> </div> </div> </li> <li> <ul> <li>to read (a kanji) with its native Japanese reading</li> </ul> <div> <div> <div>Note</div> <div>also written as 訓む</div> </div> </div> </li> </ol> </div>

3.0.0-beta.2 (Expected)

2024-07-02T02:21:59,066791377+08:00

Result on this PR

2024-07-02T02:21:07,335303148+08:00

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Todo
Development

Successfully merging this pull request may close these issues.

9 participants