use_decorated_box seems to be wrong: Its fix is not equivalent and changes UI

[fzyzcjy]

Hi thanks for the linter! I use linter a lot, and when the use_decorated_box suggests me to replace Container with DecoratedBox I did that. Later I realized it has bugs.

A very simple reproducible sample is like:

Container(
  decoration: BoxDecoration(
    border: Border.all(width: 1),
  ),
  child: MyChildWidget(),
)

With its auto-fixed DecoratedBox version:

DecoratedBox(
  decoration: BoxDecoration(
    border: Border.all(width: 1),
  ),
  child: MyChildWidget(),
)

Suppose originally the page is 100x100 pixel. Then, the MyChildWidget in Container version will be 99x99, while that in DecoratedBox version will be 100x100 - no padding is automatically added to avoid the children to collide with the decoration! This caused some of my UI to have subtle bugs later on.

By the way, we can confirm this when looking at source code of Container:

  EdgeInsetsGeometry? get _paddingIncludingDecoration {
    if (decoration == null || decoration!.padding == null)
      return padding;
    final EdgeInsetsGeometry? decorationPadding = decoration!.padding;
    if (padding == null)
      return decorationPadding;
    return padding!.add(decorationPadding!);
  }

Widget build() {
...
    final EdgeInsetsGeometry? effectivePadding = _paddingIncludingDecoration;
    if (effectivePadding != null)
      current = Padding(padding: effectivePadding, child: current);

    if (decoration != null)
      current = DecoratedBox(decoration: decoration!, child: current);
...
}

So, if Container.decoration exists, it will automatically extra pad itself.

[pq]

/fyi @minhqdao

[minhqdao]

You're right, the Container is designed to be convenience widget that does some extra things internally. In this case, it adds padding to its child to account for the size of the Decoration. To achieve the same with a DecoratedBox, you'd need to wrap its child with a Padding:

final decoration = BoxDecoration(border: Border.all(width: 2));

DecoratedBox(
  decoration: decoration,
  child: Padding(
    padding: decoration.padding!,
    child: const MyChild(),
  ),
)

Nevertheless, this means that a DecoratedBox is not an exact replacement for a Container when only decoration was provided. Not quite sure what we should do with the linting rule then. 🤔

[fzyzcjy]

@minhqdao Thanks for the reply.

Not quite sure what we should do with the linting rule then. 🤔

Well, maybe deprecate this lint? I really did not expect an official lint with seemingly trivial fix will lead to such behavior

[PawlikMichal25]

Related: #3344

[srawlins]

CC @goderbauer

Is this still a sound lint rule? should we just make the quick-fix better?

[goderbauer]

In its current form, the lint rule is not great and we've refrained from using it in the framework:

https://github.com/flutter/flutter/blob/980b5a1976883ea99301b6c6205204841ff565e8/analysis_options.yaml#L237

IMO, the rule itself doesn't provide a ton of value and contains this subtile padding foot gun. So I'd be in favor of just deprecating it. If we really want to salvage it, we'd have to improve its documentation around the padding issue (and also update its quick-fix to take that into consideration).