Add support for auto horizontal margins #879

zbarbuto · 2021-10-25T07:19:27Z

Adds support for `margin: auto` style centering.

A pretty common centering pattern in HTML/CSS is to give a left and right margin value of auto. Further, some WYSIWYG editors that output HTML will use this to center content like images instead of text-align.

As a basic example:

<p> This is some pre-text</p>
<div style="width: 150px; margin: auto; background-color: salmon;">I am centered!</div>
<p> This is some post-text</p>

Typically renders as (screenshot is from Chrome):

This PR adds support for margin: auto by changing margin storage from plain EdgeInsets to a new Margins class which wraps left, top, right, and bottom Margin values. This allows storing a raw double value as before but also to flag the particular edge as auto allowing it to be wrapped in a Row to center it if needed.

I have updated the example project with a few samples:

      <div style="width: 150px; height: 20px; background-color: red; margin: auto;">Centered Div</div>
      <div style="width: 150px; height: 20px; background-color: yellow; margin-left: auto;">margin-left: auto div</div>

Both of which now render as expected (screenshot is on an iPad):

FTR I assume ContainerSpan forms the wrapper for most elements so this would also make it work for things like img?

zbarbuto · 2021-10-25T07:41:43Z

lib/html_parser.dart

+    if(style.margin?.isAutoHorizontal ?? false) {
+      return Row(
+        mainAxisAlignment: style.margin?.alignment ?? MainAxisAlignment.start,
+        children: [Flexible(child: container)],


Have added this flexible wrapper so we don't run into issues when text overflows inside a centered div:

(blue is fixed width, red is non-fixed width, both using margin: auto)

zbarbuto · 2021-10-25T07:55:59Z

Sadly doesn't seem to work with images - any advice on what I need to change to support this? Would applying the same wrapper in _recursiveLexer be the best place?

zbarbuto · 2021-11-05T01:40:34Z

Sorry for the ping @erickok but do you have any feedback here and/or any advice for the best way I can make this work for inline images as well?

erickok · 2021-11-05T13:27:36Z

This isn't so easy to achieve I suspect but I haven't had time to take a deep dive into it due to time limitations on my side.

zbarbuto · 2021-11-08T01:41:26Z

I managed to get it working for images as well with a few tweaks:

Turns out there was a bug in the image renderer which wasn't passing the correct context (and, hence, styles) to the image which I have pull requested separately as #886

tneotia

Also when #924 is merged, there will likely be conflicts to resolve, you should be able to use the same approach as an extension on EdgeInsets to ensure non-negative values.

lib/src/css_parser.dart

zbarbuto · 2021-12-08T21:30:35Z

Have rebased on latest master.

zbarbuto · 2021-12-08T21:32:25Z

The Dart auto-formatter seems to have been a bit aggressive - is there a custom print-width I need to use or something to avoid the extra unnecessary diffs?

erickok · 2021-12-09T08:00:22Z

Unfortunaly over time we seem to have used different formatting standards (including line width) and I fear any application of a since format now would cause code changes. We should really do that one time.

tneotia · 2021-12-09T16:02:20Z

Yeah the formatter did go a bit crazy, especially in the most recent commit. I would suggest turning off auto format in VSCode and either clearing the current branch of its changes, or creating a new PR with the core changes (formatting left out).

Also this would be a breaking change in our library API as the style param would now take Margin instead of EdgeInsets.

tneotia · 2021-12-16T15:28:44Z

@zbarbuto due to all changes from the customRender PR, there are conflicts that need to be resolved now, just FYI

zbarbuto · 2021-12-20T00:52:07Z

@tneotia No problems.

Please leave this PR open for now and hopefully I'll have some time this week rebase on master without the auto-format changes rather than make a new one.

zbarbuto · 2021-12-22T23:09:37Z

@tneotia Ok I've cleaned the diffs and fixes a few issues that were inconsistent with normal HTML behaviour. The demo has been updated to show these edge-cases:

I previously was always center-aligning images that had auto horizontal margins but normal behaviour is to only do this if they are explicitly display: block (I'm guessing images are implicitly inline-block)

zbarbuto · 2021-12-22T23:13:57Z

Whoops - typo in the fuschia example - fixed:

lib/image_render.dart

tneotia

One small nit, otherwise LGTM!

tneotia

LGTM, nice work! Hopefully we can include this in the massively breaking v3.0.0... I also anticipate you'll have to do yet another rebase of this branch once #661 is merged...

tneotia · 2022-01-06T22:50:41Z

@zbarbuto #661 is merged, I promise this will be the last conflict resolve you have to do :)

Allow centering images with auto-margins

zbarbuto · 2022-01-10T01:16:44Z

I have rebased and applied my changes. There is an issue that has come up after rebase in that divs are not displaying as block-level correctly which breaks their alignment (see the coloured examples below):

For now, I am considering fixing this outside the scope of this pull request as it is currently broken on master anyway. i.e. the following:

      <div style="width: 150px; height: 20px; background-color: #ff9999;">Div A</div>
      <div style="width: 150px; height: 20px; background-color: #99ff99;">Div B</div>
      <div style="width: 150px; height: 20px; background-color: #ff99ff;">Div C</div>

Renders in a blank project using flutter_html as:

But in regular HTML e.g. in a CodePen as:

If this issue is fixed independently the alignment feature of this pull request should work as expected.

zbarbuto · 2022-02-07T21:53:45Z

@tneotia sorry for the ping but anything further I need to do on this since it has been rebased?

I can create a separate issue for the block display problem above?

tneotia · 2022-02-12T19:29:56Z

Sure I would create a separate issue. I don't think anything more needs to be done, just @erickok needs to review.

Sub6Resources · 2022-08-20T20:44:32Z

lib/style.dart

+
+  bool get isAutoHorizontal => (left is AutoMargin) || (right is AutoMargin);
+
+  Alignment? get alignment {


I like where we're going with this PR, but I'm not sure how I feel about the margin class handling its own alignment. I'm sure there are edge cases where these alignments don't hold true.

For now, can we move this alignment calculation outside of the Margins class?

I'm also going to look into how the w3 says renderers should handle auto margins for inspiration.

Thank you!

Sub6Resources · 2022-08-20T23:31:53Z

Merging this into a new branch enhancement/margin-values so I can continue refinements and get it merged in

zbarbuto force-pushed the feature/auto-margins branch from 7da128c to b15a6bf Compare October 25, 2021 07:38

zbarbuto commented Oct 25, 2021

View reviewed changes

zbarbuto force-pushed the feature/auto-margins branch from 7fc13e6 to 3aa9b0a Compare November 8, 2021 01:41

tneotia requested changes Dec 8, 2021

View reviewed changes

lib/src/css_parser.dart Outdated Show resolved Hide resolved

zbarbuto force-pushed the feature/auto-margins branch 2 times, most recently from 3d961b4 to 822d16e Compare December 8, 2021 21:30

tneotia mentioned this pull request Dec 10, 2021

Upgrade customRender to imitate customImageRender & add support for customRender for SelectableHtml #632

Merged

zbarbuto force-pushed the feature/auto-margins branch from 822d16e to 549600f Compare December 22, 2021 23:06

zbarbuto force-pushed the feature/auto-margins branch from 549600f to e47936b Compare December 22, 2021 23:13

tneotia reviewed Dec 23, 2021

View reviewed changes

lib/image_render.dart Outdated Show resolved Hide resolved

tneotia requested changes Dec 23, 2021

View reviewed changes

zbarbuto force-pushed the feature/auto-margins branch from e47936b to 735451c Compare December 23, 2021 00:07

tneotia previously approved these changes Dec 23, 2021

View reviewed changes

Add support for auto horizontal margins

c5f396d

Allow centering images with auto-margins

zbarbuto force-pushed the feature/auto-margins branch from 735451c to c5f396d Compare January 10, 2022 01:06

Sub6Resources dismissed tneotia’s stale review via c5f396d June 13, 2022 21:58

Merge branch 'master' into feature/auto-margins

bb3be56

Sub6Resources requested changes Aug 20, 2022

View reviewed changes

Sub6Resources changed the base branch from master to enhancement/margin-values August 20, 2022 23:31

Sub6Resources merged commit 65dbe79 into Sub6Resources:enhancement/margin-values Aug 20, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for auto horizontal margins #879

Add support for auto horizontal margins #879

zbarbuto commented Oct 25, 2021 •

edited

Loading

zbarbuto Oct 25, 2021

zbarbuto commented Oct 25, 2021 •

edited

Loading

zbarbuto commented Nov 5, 2021

erickok commented Nov 5, 2021

zbarbuto commented Nov 8, 2021 •

edited

Loading

tneotia left a comment

zbarbuto commented Dec 8, 2021

zbarbuto commented Dec 8, 2021 •

edited

Loading

erickok commented Dec 9, 2021

tneotia commented Dec 9, 2021

tneotia commented Dec 16, 2021

zbarbuto commented Dec 20, 2021

zbarbuto commented Dec 22, 2021

zbarbuto commented Dec 22, 2021

tneotia left a comment

tneotia left a comment

tneotia commented Jan 6, 2022 •

edited

Loading

zbarbuto commented Jan 10, 2022 •

edited

Loading

zbarbuto commented Feb 7, 2022

tneotia commented Feb 12, 2022

Sub6Resources Aug 20, 2022

Sub6Resources commented Aug 20, 2022


		bool get isAutoHorizontal => (left is AutoMargin) \|\| (right is AutoMargin);

		Alignment? get alignment {

Add support for auto horizontal margins #879

Add support for auto horizontal margins #879

Conversation

zbarbuto commented Oct 25, 2021 • edited Loading

Adds support for margin: auto style centering.

zbarbuto Oct 25, 2021

Choose a reason for hiding this comment

zbarbuto commented Oct 25, 2021 • edited Loading

zbarbuto commented Nov 5, 2021

erickok commented Nov 5, 2021

zbarbuto commented Nov 8, 2021 • edited Loading

tneotia left a comment

Choose a reason for hiding this comment

zbarbuto commented Dec 8, 2021

zbarbuto commented Dec 8, 2021 • edited Loading

erickok commented Dec 9, 2021

tneotia commented Dec 9, 2021

tneotia commented Dec 16, 2021

zbarbuto commented Dec 20, 2021

zbarbuto commented Dec 22, 2021

zbarbuto commented Dec 22, 2021

tneotia left a comment

Choose a reason for hiding this comment

tneotia left a comment

Choose a reason for hiding this comment

tneotia commented Jan 6, 2022 • edited Loading

zbarbuto commented Jan 10, 2022 • edited Loading

zbarbuto commented Feb 7, 2022

tneotia commented Feb 12, 2022

Sub6Resources Aug 20, 2022

Choose a reason for hiding this comment

Sub6Resources commented Aug 20, 2022

zbarbuto commented Oct 25, 2021 •

edited

Loading

Adds support for `margin: auto` style centering.

zbarbuto commented Oct 25, 2021 •

edited

Loading

zbarbuto commented Nov 8, 2021 •

edited

Loading

zbarbuto commented Dec 8, 2021 •

edited

Loading

tneotia commented Jan 6, 2022 •

edited

Loading

zbarbuto commented Jan 10, 2022 •

edited

Loading